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

Upgrade to Express 4.x #293

Merged
merged 5 commits into from
Jun 2, 2014
Merged

Upgrade to Express 4.x #293

merged 5 commits into from
Jun 2, 2014

Conversation

raymondfeng
Copy link
Member

"method-override": "~1.0.2",
"cookie-parser": "~1.1.0",
"morgan": "~1.1.1",
"errorhandler": "~1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK loopback should not include middleware modules in its dependencies, so that the app does not have to install middleware it does not need. That's to point of using a safe require in the wrapper object, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of them are required by LoopBack for runtime and testing. I think we need to flush out the template for scaffolded app.js first. There might be opportunities to remove or move the deps to dev/optional sections after that.

Copy link
Member

Choose a reason for hiding this comment

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

This is the minimal app.js that works:

var app = loopback();
app.use(loopback.rest());
app.listen(3000);

IMO loopback should bundle only middleware required to make the example above work, plus any middleware required by loopback custom middleware like loopback.token.

Modules required by loopback tests should be declared as dev dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

I would not worry about the app.js scaffolded by loopback-workspace, loopback-workspace can scaffold the appropriate dependency entries in package.json.

@bajtos
Copy link
Member

bajtos commented May 29, 2014

lib/express-wrapper.js is not longer a wrapper. Please rename to something more appropriate, e.g. lib/express-middlewares.js.

@bajtos
Copy link
Member

bajtos commented Jun 2, 2014

LGTM.

raymondfeng added a commit that referenced this pull request Jun 2, 2014
@raymondfeng raymondfeng merged commit 663e2d1 into 2.0 Jun 2, 2014
@raymondfeng raymondfeng deleted the feature/express-4.x branch June 2, 2014 15:26
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.

None yet

2 participants