Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

fix(build): Require correct dependencies for prod build #1855

Merged
merged 1 commit into from
Sep 6, 2017

Conversation

simison
Copy link
Member

@simison simison commented Aug 21, 2017

Some of the dependencies required by npm run start:prod aren't currently installed with npm install --production.

See #1852 for reference.

This also moves some of the require's in Gulpfile inside tasks so that they wouldn't be required if unrelated tasks are run (i.e. start:prod task).

Also changed several semver ranges from ^ to more strict ~.

@simison simison self-assigned this Aug 21, 2017
@simison simison requested review from lirantal and mleanos August 21, 2017 11:16
@simison
Copy link
Member Author

simison commented Aug 21, 2017

To test this, do:

git clone https://github.com/simison/mean.git mean-simison
cd mean-simison
git checkout restructure-prod-build
npm install --production
NODE_ENV=production npm run start:prod

@simison simison mentioned this pull request Aug 21, 2017
@simison
Copy link
Member Author

simison commented Aug 21, 2017

I'd be especially interested to have @mleanos look at those protractor changes I made! You wrote that feature originally, right?

@simison
Copy link
Member Author

simison commented Aug 21, 2017

FYI @Ghalleb tested this (#1852 (comment)) and says it seems to work fine.

Copy link
Member

@mleanos mleanos left a comment

Choose a reason for hiding this comment

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

LGTM. I've tested this locally, and deployed to Heroku using Production environment settings. I ran into an issue with our gulp default task, that I'll explain in a separate comment. However, these changes seem to accomplish their goals.

Out of curiosity, what should the production deployment flow look like? Should we run the tests?

@mleanos
Copy link
Member

mleanos commented Sep 3, 2017

I ran into an issue with our default Gulp task. It actually sets the NODE_ENV to "development". And the Procfile starts the app using gulp default. This will cause the application to run in Development mode regardless of what NODE_ENV is set to. In my case, I want my Heroku deployment to run in "production" mode.

If I remove the 'env:dev' from the default Gulp task, the application runs in whatever NODE_ENV I set in my environment variables.

@simison @Ghalleb Do you have thoughts on this?

@simison
Copy link
Member Author

simison commented Sep 6, 2017

@mleanos I'm not really familiar with Heroku. What would you recommend?

@simison
Copy link
Member Author

simison commented Sep 6, 2017

@mleanos On second thought:

  • npm start should probably just use existing NODE_ENV and default to development. In package.json scripts we could have:
'start': 'if [ $NODE_ENV == "production" ]; then npm run start:prod; else npm run start:dev; fi'

... but that test could be also at Gulpfile.js — might be even more clear to have it there? I'm not exactly sure how well bash conditionals work e.g. in Windows machines.

  • npm run start:prod and npm run start:dev stick to using `'env:' Gulp tasks.

@Ghalleb
Copy link
Contributor

Ghalleb commented Sep 6, 2017

If you look at my PR #1854, I changed npm start to npm run start:prod in the procfile as this file is tipycally used for production only...

@simison
Copy link
Member Author

simison commented Sep 6, 2017

@Ghalleb thanks! So I'll merge this and that change can be done in your PR.

@simison simison merged commit 36887dc into meanjs:master Sep 6, 2017
@simison simison deleted the restructure-prod-build branch September 6, 2017 22:49
@simison simison mentioned this pull request Sep 6, 2017
cicorias pushed a commit to JavaScriptExpert/mean that referenced this pull request Sep 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants