Define remote methods using json schema definitions. Prototype for #209

[mrfelton]

Enables remote methods to be defined in the json schema and applied automatically to the generated models. #209

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[raymondfeng]

@mrfelton It's a great addition. Can you add some test cases?

[bajtos]

Please rework this part according to #209 (comment), so that:

• key prototype.methodName is mapped to remoteMethod('methodName', { isStatic: false, ... })
• key methodName is mapped to remoteMethod('methodName', { isStatic: true, ... })

[ritch]

@bajtos please consider the workspace implementation. Lets not make it impractical to implement the generator via the workspace. I'm not against prototype.methodName itself... but we might be making more work for ourselves in the workspace without much benefit.

I'd really like to see remote method creation support added to workspace alongside this addition... These JSON files are specifically designed to serve the workspace well so this new feature is no exception.

[bajtos]

Please consider the workspace implementation. Lets not make it impractical to implement the generator via the workspace.

Can you please elaborate more why my proposal is impractical for loopback-workspace to implement? Are you afraid of parsing prototype.{name} in the workspace?

We can modify the implementation as follows if you think it will make the workspace implementation simpler:

• methodName is mapped to isStatic:true unless the config object provides a different value
• prototype.methodName is always mapped to isStatic:false, regardless of the value in the config object

// static methods
methods: { "work": { isStatic: true, /*...*/ } }
methods: { "work": { /*...*/ } }

// prototype methods
methods: { "work": { isStatic: false, /*...*/ } }
methods: { "prototype.work": { /*...*/ } }

What I really want to avoid is adding more stuff that we will have to rework when fixing #209 and thus causing #209 to introduce even more breaking changes.

[ritch]

Can you please elaborate more why my proposal is impractical for loopback-workspace to implement

The workspace uses keys as identifiers for entities (in the case a method would be an entity).

prototype.methodName is always mapped to isStatic:false, regardless of the value in the config object

I like where you are going with this... but consider what the workspace does. It loads properties, and other Entities and also writes them back to files. We would have to store a bit that says "this method was defined using prototype.methodName" and recall that during serialization. Of course this is all possible... but probably not worth it to get rid of the annoying isStatic property.

[bajtos]

Nice to have: rework built-in models in common/models/* to use this new declarative approach too.

[bajtos]

@slnode ok to test

[bajtos]

Besides adding tests, we also need you @mrfelton to sign our CLA: https://cla.strongloop.com/agreements/strongloop/loopback

[ritch]

I've been wanting to add this for quite some time... 👍

[mrfelton]

Have added a basic test case to verify that remote methods defined via the json spec are properly added to the model. As for the handling of static vs prototype methods, I feel like that should be handled separately in #597 as it's a much wider change that would affect far more than what I was trying to do in #209 (making it possible to define remote methods in json).

[bajtos]

As for the handling of static vs prototype methods, I feel like that should be handled separately in #597 as it's a much wider change that would affect far more than what I was trying to do in #209 (making it possible to define remote methods in json).

I really don't want people to start adding JSON metadata in the format that is already known to get changed in the near future. All I am asking is to modify the piece of code calling ModelCtor.remoteMethod to pre-process the key, no other changes are needed.

@ritch what's your opinion on this? If you agree with me, then I can add the necessary code on top of @mrfelton's patch to get the desired API.

[ritch]

I really don't want people to start adding JSON metadata in the format that is already known to get changed in the near future.

Agreed. Lets not be hasty in adding this to the format... there's a lot of downstream work when adding anything to the $model.json file. I'm -1 for landing this without at least an initial PR that adds support for it in the workspace.

Regarding methods: { "prototype.work": { /*...*/ } }. Why not leave this off as it is entirely additive (in your last proposal @bajtos)? Although I am fine having this style supported (if it is also supported in the workspace PR mentioned above).

[bajtos]

Regarding methods: { "prototype.work": { /.../ } }. Why not leave this off as it is entirely additive (in your last proposal @bajtos)?

Because it's not additive, it's a breaking change as explained in #597. ATM, if you don't specify isStatic, it defaults to false, i.e. { methods: { "work": { /*...*/ } } } is interpreted as a prototype method in the implementation proposed by this pull request.

What I would like to see is that { methods: { "work": { /*...*/ } } } is interpreted as a static method.

Lets not be hasty in adding this to the format... there's a lot of downstream work when adding anything to the $model.json file. I'm -1 for landing this without at least an initial PR that adds support for it in the workspace.

I understand your concerns, we are building technical debt in the workspace API. On the other hand, I feel it's not fair to ask people contributing useful loopback core features to implement support for them in loopback-workspace too.

[bajtos]

@ritch what's your opinion on my comment above? It would be nice to come to an agreement to unblock progress of this pull request.

If you are still not convinced about my proposal, then how these two alternatives?

1. Make isStatic required, don't provide (broken) default value.
2. Make isStatic default to true, this will allow us to add support for prototype.name later and in a way that does not break compatibility.

[bajtos]

@ritch ping, this pull request is waiting for your comment, can you please read #1163 (comment) and #1163 (comment) and let me know what solution do you consider as the best (or at least acceptable to you)?

[ritch]

Make isStatic required, don't provide (broken) default value.

Once we add the nicer syntax we can make this not required if you specify the sugary syntax (prototype.foo). So I am in favor of this option to move this forward.

[ritch]

I feel it's not fair to ask people contributing useful loopback core features to implement support for them in loopback-workspace too

I understand that its not fair to say "Thanks for the contribution, but we won't accept it unless you add support in x, y and z". But you also have to understand (as someone contributing to core) that their are upstream / downstream projects that were purposefully developed concurrently with loopback core to ensure that once a capability was designed it would function throughout the entire platform.

In this case it is not a matter of technical debt. Its a matter of compatability. The reason we have these JSON files in the first place is because of the workspace. We've been bitten in the past by hastily designing syntax (cough.. ACLs cough...) without thinking about its use in JSON files. What we are talking about here is the other way around sure (we've got some good JSON syntax ideas) but we cannot gloss over the fact that this will be parsed by the workspace API, and eventually implemented in the composer UI.

[bajtos]

Closing in favour of #1366.