Ensure npm run build is in production mode

[kristw]

Using --mode=production does not work with some modules. In particular, react-hot-loader uses NODE_ENV to decide whether to use empty shell or HMR code.

See bundle size before / after this change.

p.s. I tried using webpack.DefinePlugin({ 'process.env.NODE_ENV': mode }) to avoid hardcoding env variable in npm script but was not successful, so I settle with this.

@williaster @xtinec

[mistercrunch]

LGTM

[codecov-io]

Codecov Report

Merging #5934 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5934   +/-   ##
=======================================
  Coverage   48.19%   48.19%           
=======================================
  Files         393      393           
  Lines       23600    23600           
  Branches     2630     2630           
=======================================
  Hits        11375    11375           
  Misses      12225    12225

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71f014e...fe0d87e. Read the comment docs.

[xtinec]

argv.mode might do the trick.
https://github.com/webpack/webpack/issues/7074#issuecomment-393108512

If not, the current solution works as well.

[kristw]

@xtinec The check happens in react-hot-loader code which I cannot modify.
https://github.com/gaearon/react-hot-loader/blob/master/index.js

[xtinec]

@kristw I was thinking something like this in webpack.config.js. I'm not 100% sure if it would work, this seems to suggest it may. Figured it may be worth a try. 😄

module.exports = (env, argv) => ({
	...etc,
	plugins: [
           ...etc,
           new webpack.DefinePlugin({
             'process.env.WEBPACK_MODE': JSON.stringify(mode),
             'process.env.NODE_ENV': argv.mode
           })]
});

If not, totally cool with leaving as-is. 👍

[kristw]

@xtinec Thanks for suggesting. I tried but didn't work.

BTW mode from the line above is actually same with argv.mode. I parsed argv via minimist

[williaster]

should we also add this to the prod script? or delete this script? @mistercrunch do you know why this is here? airbnb doesn't use it, and iirc it was just setting an upper limit on the process, which also makes it take like 20 min.

[mistercrunch]

prod is legacy at this point, we use build, I don't fully understand the implications / differences...

[xtinec]

@kristw Aha, got it. Thanks for clarifying! 👍 The change looks good to go. 🚢

[kristw]

I can do whatever you both decide. Please let me know. I don't know what prod is for, so didn't touch it.

Thanks @xtinec . Always great to have second eye on.

[xtinec]

I found an old PR from 2016 that reduced the size to what it is today.
#1679

Seems like there is no harm deleting it as the reasoning is weakened today with RAMs getting bigger. 😄

We should probably update this bit in CONTRIBUTING.md before deleting prod and also make a note in UPDATING.md in case anyone has npm/yarn run prod in their build pipeline.

# Compile the Javascript and CSS in production/optimized mode for official releases
npm run prod

Thoughts? @mistercrunch @williaster @kristw

I'm also cool with this PR shipping as-is. Going to go ahead and approve now.

[mistercrunch]

Last I checked the tradeoffs between run build and run prod were that run prod took much longer and was significantly smaller. I'm not sure exactly what it entails.

[kristw]

Umm, how about merging this and make dropping npm run prod as another PR so it will be more obvious.

[williaster]

sgtm