Skip to content

Update sdk to use loopback 3.x - #253

Merged
davidcheung merged 1 commit into
masterfrom
use-lb3
Jan 4, 2017
Merged

Update sdk to use loopback 3.x#253
davidcheung merged 1 commit into
masterfrom
use-lb3

Conversation

@davidcheung

Copy link
Copy Markdown
Contributor

Comment thread lib/services.template.ejs
// this is to handle strong-remoting does not prepend prototype.
// back into prototype methods
if (action.sharedMethod.isStatic === false) {
aliasMethod = ['prototype', aliasMethod].join('$');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think aliases of prototype methods should be handled by strong-remoting. I thought we have already discussed this, was there a good reason why to keep the above workaround?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos previous convo conclusion is to just prepend prototype to avoid a breaking change in strong-remoting

bajtos: well, we can introduce a method to get a list of all methods & aliases
but e.g. the rest adapter does not want to de-duplicate aliases
david: right, ill take a look and fix this problem with patchAttributes, then maybe add an issue to see if we should explore " introduce a method to get a list of all methods & aliases"
bajtos: hmm, I just realised my proposal would be a breaking change in strong-remoting
because code expecting to see aliases without prototype. prefix will break after the change
so maybe it will be best to fix the problem in Angular SDK only for short-termuce a method to get a list of all methods & aliases" for longer term

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, makes sense 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please create an issue to keep track of the long-term solution?

Comment thread test.e2e/test-server.js
var options = models[m].options || {};
if (initialModels[m]) {
model = initialModels[m];
lbApp.model(model, extend({ dataSource: 'db' }, options));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading this code correctly, then you are calling lbApp.model twice for any custom model that happens to be in initialModels too.

I think a better solution is to merge initialModels to models so that you can iterate only over models here.

Comment thread test.e2e/test-server.js Outdated
loopback.autoAttach();

if (enableAuth)
lbApp.enableAuth();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use lbApp.enableAuth({ dataSource: 'db' }), it will auto attach any required models that are not attached yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be actually the only change needed to fix removed autoAttach call.

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL ☝️

@davidcheung

Copy link
Copy Markdown
Contributor Author

@slnode test please

@davidcheung

davidcheung commented Nov 25, 2016

Copy link
Copy Markdown
Contributor Author

looks like cis-jenkins picks up the dependency same as npm

npm info loopback@^3.0.0 --registry=http://apiconnect01.rchland.ibm.com:4873/ .version .gitHead
.version = '3.0.0'
.gitHead = '8d0f319dd616a047793f8e8bc02f0649a97afdff'

strongloop/loopback@8d0f319

@bajtos

bajtos commented Dec 5, 2016

Copy link
Copy Markdown
Member

@slnode test please

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM, now we need to figure out why cis-jenkins is failing...

@bajtos

bajtos commented Dec 5, 2016

Copy link
Copy Markdown
Member

looks like cis-jenkins picks up the dependency same as npm

I have released loopback@3.1.0 this morning, and restarted the build few moments ago. Let's see if it's still failing.

@davidcheung

Copy link
Copy Markdown
Contributor Author

@slnode test please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants