Skip to content
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.

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

[SEMVER-MAJOR] Rework the module to a loopback component #104

Merged
merged 1 commit into from Aug 10, 2015

Conversation

@bajtos
Copy link
Member

commented Jul 1, 2015

Rework the exported function to conform to the component convention:

var loopback = require('loopback');
var explorer = require('loopback-explorer');
var app = loopback();
explorer(app, options);
  • drop dependency on express
  • drop support for loopback 1.x
  • add a new option "mountPath" to specify where to mount the explorer UI
    and swagger metadata

Close #21

/to @raymondfeng @ritch please review
/cc @STRML

@STRML

This comment has been minimized.

Copy link
Member

commented Jul 1, 2015

Understood why this is being done, two questions:

  1. Isn't loopback support actually >=1.9.0, not >=2.0, as mentioned here?
  2. Why add mountPath at all? I think the existing app.use('/explorer', explorer(app, options)) syntax is much easier to understand, easier to change, and by keeping it we won't break any existing apps. Otherwise we now have a breaking change for all apps without much benefit.

@bajtos bajtos changed the title Rework the module to a loopback component [SEMVER-MAJOR] Rework the module to a loopback component Jul 2, 2015

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2015

@STRML thank you for the comment. I should have made it more clear that this pull request is a breaking change that will be released as a new major version. I modified the PR title to reflect that, hopefully it's more clear now.

The current implementation of explorer is a sort of a middleware (which typically accepts options only and is mounted on the app) and a sort of a component (which typically accepts app and options and modifies the app in any way it wishes). This causes problems with declarative registration via middleware.json, because there is no way how to specify that an app object should be passed in middleware factory arguments explorer(app, options). IMO, that's actually correct - middleware factory should not require an app object to create the middleware function.See strongloop/loopback-boot#75 for the related discussion.

We were discussing this with @ritch and @raymondfeng and came to the conclusion that it will be best to rework explorer into a component. The signature of explorer(app, options) already follows the signature of component functions. This way the explorer can be configured via component-config.json:

// server/component-config.json
{
  "loopback-explorer": {
      "mountPath": "/explorer"
   }
}

Isn't loopback support actually >=1.9.0, not >=2.0

Since we are already making other breaking changes in this pull request, I thought this is the best time to drop support for LoopBack 1.x, so that we don't have to deal with express 3.x vs 4.x differences.

Hopefully my intentions are more clear now. What's your opinion on this direction, @STRML? Do you still think the benefits are not worth the breaking change?

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2015

@STRML ping, what's your opinion on this patch? Can we land it?

@STRML

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

It makes sense re: your move toward JSON configuration. I'm not a big fan of configuration over code but so long as the docs make the contents of the options object explicit I think it will be fine.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2015

@STRML I have an idea how to make the migration easier: what if the module exports a routes function that can be used in the old 1.x way?

app.use('/explorer', explorer.routes(app, options))

The new component-centric API can be implemented as a thin wrapper around this new routes function:

module.exports = function(app, options) {
  app.use(options.mountPath, routes(app, options));
}

Thoughts?

@STRML

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

I like that. It makes for a very short entry in the migration doc.

:shipit:

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

LGTM

@raymondfeng raymondfeng assigned bajtos and unassigned raymondfeng Aug 9, 2015

@bajtos bajtos force-pushed the feature/refactor-as-component branch from d772e34 to ffe29e2 Aug 10, 2015

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2015

Refactored to explorer.routes(app, options), as discussed above.

@STRML LGTY now?

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2015

@slnode test please

@STRML

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

:shipit:

Rework the module to a loopback component
Rework the exported function to conform to the component convention:

    var loopback = require('loopback');
    var explorer = require('loopback-explorer');
    var app = loopback();
    explorer(app, options);

Allow users to mount explorer as a middleware too:

    app.use('/explorer', explorer.routes(app, options));

- drop dependency on express
- drop support for loopback 1.x
- add a new option "mountPath" to specify where to mount the explorer UI
  and swagger metadata
- describe upgrading from v1.x in README

@bajtos bajtos force-pushed the feature/refactor-as-component branch from ffe29e2 to 7ee8703 Aug 10, 2015

bajtos added a commit that referenced this pull request Aug 10, 2015
Merge pull request #104 from strongloop/feature/refactor-as-component
[SEMVER-MAJOR] Rework the module to a loopback component

@bajtos bajtos merged commit 75ea3f9 into master Aug 10, 2015

2 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
default Build finished. No test results found.
Details

@bajtos bajtos deleted the feature/refactor-as-component branch Aug 10, 2015

@bajtos bajtos removed the #review label Aug 10, 2015

@rmg

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

@bajtos when making breaking changes like this in a PR it is important that the PR includes the actual version bump, otherwise the breaking change gets pushed to our staging registry using the current major version number and everything breaks.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2015

@rmg oh, I did not realise the implication for our CI system, sorry for the troubles I have caused. I will bump up major versions in the future.

For posterity, 8c82f17 fixed the issue for this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.