Skip to content

Improve remoting metadata: register exported models using singular names - #151

Merged
bajtos merged 4 commits into
masterfrom
feature/improve-remoting-metadata
Jan 27, 2014
Merged

Improve remoting metadata: register exported models using singular names#151
bajtos merged 4 commits into
masterfrom
feature/improve-remoting-metadata

Conversation

@bajtos

@bajtos bajtos commented Jan 21, 2014

Copy link
Copy Markdown
Member

The first commit is just a cleanup that was made possible by strongloop/strong-remoting#37.

The second change is the purpose of this pull request:

Remove the inconsistency between model names used by LoopBack app and
datasource-juggler (modelName, e.g. User) and the name used by
strong-remoting (pluralModelName, e.g. Users).

This way the class name in the strong-remoting metadata can be used
by client-code generators. Before this change, the generators would
produce method names like Users.login.

Important note

This patch is introducing several incompatibilities with previous versions:

  1. Shared models are no longer exported using plural names. Any code expecting plural keys in app.remotes().exports will no longer work.
  2. app.remoteObjects() uses singular names for the keys. Any code expecting plural keys will no longer work.
  3. Since remoting hooks are using class name, any code bypassing ModelClass.beforeRemote/afterRemote and registering hooks direcly via app.remotes().before/after will no longer works.

Code using the keys of app.remotes().exports or app.remoteObjects() to build remotable method names (e.g. key + '.create' will continue to work.

/to: @ritch or @raymondfeng please review

To follow semver rules, we should release this change as LoopBack 2.0. Any objections?

@bajtos

bajtos commented Jan 21, 2014

Copy link
Copy Markdown
Member Author

@ritch

ritch commented Jan 21, 2014

Copy link
Copy Markdown
Member

Any way this can be backwards compatible? 2.0 will make it hard for people to get fixes.

bajtos referenced this pull request in loopbackio/loopback-datasource-juggler Jan 21, 2014
Set the HTTP route to `'/' + pluralModelName` so that we don't have
to duplicate this bit of logic in strong-remoting and other places.
@slnode

slnode commented Jan 21, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/828/

@bajtos

bajtos commented Jan 21, 2014

Copy link
Copy Markdown
Member Author

Any way this can be backwards compatible? 2.0 will make it hard for people to get fixes.

I will try to come up with some kind of a compatibility layer tomorrow. The difficult part is how to avoid duplicate entries. Perhaps strong-remoting could support some sort of alias for class names.

On the other hand, the parts affected by this change are sort of internal and not documented well (if at all), which means the number of affected users should be very small. I guess could post a question on our mailing list to find out if there is anybody using these features.

I will probably make the compatibility layer turned off by default. The idea is to make sure there is a quick solution if somebody needs it, but don't let new users use the legacy APIs.

@slnode

slnode commented Jan 22, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/837/

@bajtos

bajtos commented Jan 22, 2014

Copy link
Copy Markdown
Member Author

@ritch I have added another commit that introduces loopback.compat.usePluralNamesForRemoting.

The idea is that after our user upgrades from v1.x to v2.x and finds out that his application no longer works, s/he can add one line to his app.js file to get the good old behaviour:

// app.js
loopback.compat.usePluralNamesForRemoting = true;

This way they can use the new features & bugfixes from 2.x versions without having to immediately rewrite their application.

Maybe we can release this change as v1.6 instead of v2.0? As I wrote earlier, I don't expect there will be many people affected by this change (if any at all).

@slnode

slnode commented Jan 22, 2014

Copy link
Copy Markdown

Test PASSed. To trigger a build add comment - ".test\W+please"
Refer to this link for build results: http://ci.strongloop.com/job/loopback/839/

@bajtos

bajtos commented Jan 24, 2014

Copy link
Copy Markdown
Member Author

As I was implementing the Angular code generator, I realised it's possible to find the singular model name somewhere deep in the metadata object tree. So this pull request is not strictly necessary.

This is the question we should consider: Are we happy with the inconsistent class names (singular in LoopBack and app, plural in strong-remoting)?

I am not, but you may have a different opinion.

If we agree that the class names should be consistent, then it's best to make the change now, until the number of affected users is low. If we don't mind the inconsistency, then this pull request can be closed and I'll implement a workaround in the code gen.

/to: @ritch @raymondfeng
/cc: @Schoonology you may have an opinion on this too.

@ritch

ritch commented Jan 24, 2014

Copy link
Copy Markdown
Member

+1 for consistency

@raymondfeng

Copy link
Copy Markdown
Member

+1 for consistency too. Otherwise, the changes LGTM.

@seanbrookes

Copy link
Copy Markdown

+1 for consistency.

this is the kind of thing can really come back to haunt

On Fri, Jan 24, 2014 at 8:21 AM, Raymond Feng notifications@github.comwrote:

+1 for consistency too. Otherwise, the changes LGTM.


Reply to this email directly or view it on GitHubhttps://github.com//pull/151#issuecomment-33236826
.

@ritch

ritch commented Jan 24, 2014

Copy link
Copy Markdown
Member

merge it 👍

@ritch

ritch commented Jan 24, 2014

Copy link
Copy Markdown
Member

Note: we should release this as 1.6.0.

@Schoonology

Copy link
Copy Markdown
Contributor

No qualms here. :shipit:

Miroslav Bajtoš added 4 commits January 27, 2014 10:29
Use the new property introduced by a recent change in
loopback-datasource-juggler instead of building the URL
manually from pluralModelName.
Remove the inconsistency between model names used by LoopBack app and
datasource-juggler (modelName, e.g. User) and the name used by
strong-remoting (pluralModelName, e.g. Users).

This way the class name in the strong-remoting metadata can be used
by client-code generators. Before this change, the generators would
produce method names like `Users.login`.
Add a compatibility layer that allows applications based on LB pre-v1.6
to work with 1.6 versions with a minimum amount of changes required.

New flag(s):
  compat.usePluralNamesForRemoting
strong-remoting ~1.2.1
loopback-datasource-juggler ~1.2.13
@bajtos

bajtos commented Jan 27, 2014

Copy link
Copy Markdown
Member Author

I have reworded 20766e2 to use 1.6 instead of 2.x, bumped up dependencies in 4699219 and rebased everything on top of the current master.

bajtos added a commit that referenced this pull request Jan 27, 2014
…data

Improve remoting metadata: register exported models using singular names
@bajtos
bajtos merged commit fa0f402 into master Jan 27, 2014
@bajtos
bajtos deleted the feature/improve-remoting-metadata branch January 27, 2014 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants