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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separating JS dev and prod dependencies #3241

Merged
merged 5 commits into from May 28, 2019

Conversation

@patjouk
Copy link
Collaborator

commented May 27, 2019

We're installing a bunch of JS deps that are not necessary for prod. I tried to separate dev/prod dependencies, but I might have missed one or two, can someone double check? I'm pretty sure using NODE_ENV=production for review apps, staging and prod will be okay, but again, correct me if I'm wrong 馃槄 It's already running with this setting, never mind :D

It won't change anything for local dev or CI: all dependencies will still be installed when running npm i or building the node image. Only review apps, staging and prod will run without dev dependencies.

todos:

  • Add NPM_CONFIG_PRODUCTION=true to staging,
  • Add NPM_CONFIG_PRODUCTION=true to prod.

@patjouk patjouk added the engineering label May 27, 2019

@patjouk patjouk requested review from cadecairos, mmmavis and youriwims May 27, 2019

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3241 May 27, 2019 Inactive

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3241 May 27, 2019 Inactive

@patjouk

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2019

Heroku was still installing all dependencies. I checked the doc: we need to use NPM_CONFIG_PRODUCTION=true and not NODE_ENV=production.

@cadecairos cadecairos had a problem deploying to foundation-mofostaging-pr-3241 May 27, 2019 Failure

"eslint": "^5.16.0",
"eslint-config-prettier": "^4.3.0",
"eslint-plugin-prettier": "^3.1.0",
"eslint-plugin-react": "^7.13.0",
"event-stream": "3.3.4",
"html-entities": "^1.2.1",
"js-cookie": "2.2.0",
"mofo-bootstrap": "4.1.0",
"mofo-style": "^2.4.0",

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 27, 2019

Member

Seems like we no longer uses mofo-style. I'll file a separate ticket to remove this.

This comment has been minimized.

Copy link
@mmmavis
@mmmavis
Copy link
Member

left a comment

Looks good!

Maybe @cadecairos can chime in as I'm not super familiar with Heroku set up - do we need to have both NPM_CONFIG_PRODUCTION=true and NODE_ENV=production for every Heroku app (review apps + staging + prod)?

@youriwims

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

I'll let Mavis & Cade review this since I'm not super knowledgeable on all the packages we utilize. 馃槄

@youriwims youriwims removed their request for review May 27, 2019

@cadecairos
Copy link
Member

left a comment

looks good, was able to install and run without issue.

@cadecairos cadecairos had a problem deploying to foundation-mofostaging-pr-3241 May 28, 2019 Failure

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3241 May 28, 2019 Inactive

@patjouk patjouk merged commit c4b29be into master May 28, 2019

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 78.132%
Details
percy/foundation.mozilla.org Visual review automatically approved, no visual changes found.
Details

@patjouk patjouk deleted the npm-dev branch May 28, 2019

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鈥檛 perform that action at this time.