Skip to content

Update project to remove isStatic flag #273

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
merged 1 commit into from
May 27, 2016
Merged

Update project to remove isStatic flag #273

merged 1 commit into from
May 27, 2016

Conversation

0candy
Copy link
Contributor

@0candy 0candy commented Apr 7, 2016

@0candy 0candy self-assigned this Apr 7, 2016
@0candy 0candy added the #review label Apr 7, 2016
@0candy 0candy assigned bajtos and unassigned 0candy Apr 7, 2016
@bajtos
Copy link
Member

bajtos commented Apr 8, 2016

Only for LB 3.0

There is no LB 3.0 version of loopback-workspace. We need the same version of loopback-workspace to support both LB 2.x and LB 3.x applications.

bajtos
bajtos reviewed Apr 8, 2016
View reviewed changes
common/models/model-method.json
@@ -18,9 +18,6 @@
"aliases": {
"type": "array"
},
"isStatic": {
"type": "boolean"
},
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep this property around, so that consumers of loopback-workspace do not need to understand how exactly are remote methods described in JSON files. That way the API of loopback-workspace remains the same, which means less work for us too.

@bajtos bajtos mentioned this pull request Apr 8, 2016
Merged
@bajtos bajtos assigned 0candy and unassigned bajtos Apr 8, 2016
@0candy 0candy force-pushed the update_remote_method branch 2 times, most recently from 475e51d to 318f04b Compare April 13, 2016 03:39
@0candy 0candy assigned bajtos and unassigned 0candy Apr 13, 2016
bajtos
bajtos reviewed Apr 13, 2016
View reviewed changes
common/models/model-method.js
@@ -2,6 +2,8 @@
// Node module: loopback-workspace
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT
var app = require('../../server/server');
Copy link
Member

Choose a reason for hiding this comment

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

Models in "common" should not depend on things in "server", that goes against the direction of dependencies. You can access the app via ModelMethod.app after the model was configured (attached to a datasource & app).

@bajtos bajtos assigned 0candy and unassigned bajtos Apr 13, 2016
@0candy 0candy force-pushed the update_remote_method branch 5 times, most recently from 4b495f3 to 6a7e8e3 Compare April 14, 2016 23:31
@0candy 0candy assigned bajtos and unassigned 0candy Apr 14, 2016
bajtos
bajtos reviewed Apr 18, 2016
View reviewed changes
common/models/model-definition.js
result = {};
relatedData.forEach(function(related) {
var key = related[relation.embed.key];
var Entity = require('loopback').getModel(relation.model);
Copy link
Member

Choose a reason for hiding this comment

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

Please use app.models[relation.model] to stay consistent with L65 above.

bajtos
bajtos reviewed May 18, 2016
View reviewed changes
common/models/model-definition.js
var keyFn = relation.embed.keyFn;
if (keyFn && typeof Definition[keyFn] === 'function') {
key = Definition[keyFn](related);
var keySetter = relation.embed.keySetter;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be keyGetter, because we want to build a key from the data stored in our domain entity, shouldn't it?

@bajtos bajtos assigned 0candy and unassigned bajtos May 18, 2016
@0candy 0candy force-pushed the update_remote_method branch 3 times, most recently from 01b66a0 to 007306b Compare May 19, 2016 15:28
@0candy 0candy assigned bajtos and unassigned 0candy May 20, 2016
bajtos
bajtos reviewed May 24, 2016
View reviewed changes
common/models/definition.js
@@ -128,12 +128,13 @@ module.exports = function(Definition) {
fileData.name, embedId, config);
}

config = Entity.getDataFromConfig(config);
config = Entity.getDataFromConfig(config, embedId);

// add extra properties for relations
config[relation.foreignKey] = fk;
config[relation.embed.key] = embedId;
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to handle the case when relation.embed.key is not defined, because there are embed.keySetter and embed.keyGetter instead.

@bajtos bajtos assigned 0candy and unassigned bajtos May 24, 2016
@0candy 0candy assigned bajtos and unassigned 0candy May 25, 2016
@bajtos
Copy link
Member

bajtos commented May 26, 2016

@0candy LGTM. Please check the CI build failure on amazon-0.12 - link and squash the commits before landing.

I am going to restart CI to see if the problem was temporary.

@bajtos
Copy link
Member

bajtos commented May 26, 2016

@slnode test please

@bajtos bajtos assigned 0candy and unassigned bajtos May 26, 2016
@0candy 0candy force-pushed the update_remote_method branch from 1d3ee29 to 82fd603 Compare May 27, 2016 16:14
@0candy
Copy link
Contributor Author

0candy commented May 27, 2016 •
edited
Loading

amazon-0.12 is not failing after you restarted. The only thing failing now is generator-loopback and that's waiting to be fixed.

@0candy 0candy merged commit 2cfab28 into master May 27, 2016
@0candy 0candy deleted the update_remote_method branch May 27, 2016 17:42
@0candy 0candy removed the #review label May 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@0candy 0candy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0candy @bajtos