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
Merged

Separating JS dev and prod dependencies #3241

merged 5 commits into from May 28, 2019

Conversation

patjouk
Copy link
Contributor

@patjouk patjouk 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.

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3241 May 27, 2019 14:32 Inactive
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3241 May 27, 2019 14:49 Inactive
@patjouk
Copy link
Contributor Author

patjouk 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 14:58 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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Filed: #3248

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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 16:37
Copy link

@cadecairos cadecairos left a comment

Choose a reason for hiding this comment

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

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

@cadecairos cadecairos had a problem deploying to foundation-mofostaging-pr-3241 May 28, 2019 07:52 Failure
@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3241 May 28, 2019 08:07 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants