Skip to content

Use strong-remoting's new TypeRegistry #2696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
bajtos merged 3 commits into from
Sep 9, 2016
Merged

Use strong-remoting's new TypeRegistry #2696

bajtos merged 3 commits into from
Sep 9, 2016

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Aug 31, 2016 •
edited by richardpringle
Loading

The first two commits fix issues discovered by strong-remoting's duplicate checker.

The last commit reworks model registration to use the new strong-remoting API.

Requires strongloop/strong-remoting#343

to: @gunjpan @richardpringle @deepakrkris Please review.
cc: @ritch @STRML @0candy @davidcheung

Connect to strongloop-internal/scrum-loopback#885

TODO before merging

@bajtos bajtos added this to the #Epic: Coercion Cleanup milestone Aug 31, 2016
@bajtos bajtos added the #review label Aug 31, 2016
This was referenced Aug 31, 2016
Merged
Merged
@bajtos
Copy link
Member Author

bajtos commented Aug 31, 2016

@slnode test please

@bajtos
Copy link
Member Author

bajtos commented Aug 31, 2016

@0candy actually I think you were right, the CI is failing because it installs strong-remoting version that does not contain strongloop/strong-remoting#343

>> TypeError: this.remotes(...).defineObjectType is not a function
>>   at EventEmitter.app.model (D:\workspace\loopback\68dfec3a\lib\application.js:155:20)

ritch
ritch reviewed Sep 7, 2016
View reviewed changes
lib/application.js
this.models().push(Model);

if (isPublic && Model.sharedClass) {
this.remotes().defineObjectType(Model.modelName, function(data) {
Copy link
Member

@ritch ritch Sep 7, 2016

Choose a reason for hiding this comment

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

Ah this is what I was missing

@Amir-61
Copy link
Member

Amir-61 commented Sep 9, 2016 •
edited
Loading

Hey Miroslav,

Once merge conflicts are solved I would also run tests locally to make sure with npm link with your branch in strongloop/strong-remoting#343 they all pass.

  • Did you leave the changes with three commits on purpose? Shouldn't we squash them as one single commit?

@bajtos
app.enableAuth: correctly detect attached models
32bdecc 
Fix a typo in "app.enableAuth" that caused the method to not detect
the situation when e.g. the built-in User model is already attached
to a datasource.
@bajtos
test/user: don't attach User model twice
92a5a08
@bajtos bajtos mentioned this pull request Sep 9, 2016
Merged
3 tasks
@bajtos bajtos force-pushed the feature/coercion-overhaul branch from 8262059 to 6e1defc Compare September 9, 2016 08:01
@bajtos
Copy link
Member Author

bajtos commented Sep 9, 2016

Did you leave the changes with three commits on purpose? Shouldn't we squash them as one single commit?

Yes, I left multiple commits purposefully. each of the commit fixes a problem that's not related to the new type registry, even though it was discovered while making the upgrade.

@bajtos
Copy link
Member Author

bajtos commented Sep 9, 2016

test please

@bajtos
Copy link
Member Author

bajtos commented Sep 9, 2016

Windows CI is failing because PhantomJS cannot start, everything else is green now. I am going ahead and will land the patch.

@bajtos bajtos merged commit 252b6f4 into master Sep 9, 2016
@bajtos bajtos deleted the feature/coercion-overhaul branch September 9, 2016 08:34
@bajtos bajtos removed the #review label Sep 9, 2016
@richardpringle richardpringle removed their assignment Sep 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@deepakrkris deepakrkris

@gunjpan gunjpan

Labels

None yet

Projects

None yet

Milestone

#Epic: Coercion Cleanup

Development

Successfully merging this pull request may close these issues.

10 participants

@bajtos @Amir-61 @ritch @airazabal @richardpringle @abhullar @deepakrkris @0candy @gunjpan