Set mode and NODE_ENV defaults

[eliperelman]

Fixes #900.

This is just an initial patch, needs a review to push this forward more.

[edmorley]

I have a branch locally where I've added a few more changes on top of this PR, however I think it might be useful to discuss the expected UX for the various combinations of options.

A) --mode {production,development} and NODE_ENV undefined:

--> Use the specified value for both mode and NODE_ENV

B) --mode 'none' and NODE_ENV undefined:

Do we even need to support this case? Possible options:

1. throw new Error('mode "none" is not supported')
2. leave NODE_ENV undefined
3. set NODE_ENV to 'development'
4. set NODE_ENV to 'production'
5. set NODE_ENV to 'none'

C) no --mode passed and NODE_ENV undefined:

1. throw new Error('Either --mode or NODE_ENV must be set')
2. set both mode and NODE_ENV to 'production'
   • This is would be more consistent with webpack's mode default than option 3.
3. set both mode and NODE_ENV to 'development'

I think I prefer option 1 (Zen of Python "explicit is better than implicit" and all that), followed by option 2.

D) no --mode specified and NODE_ENV={production,development}:

--> Set mode to the same value as NODE_ENV

E) no --mode specified and NODE_ENV={test,<anything else>}:

1. set mode to 'development'
2. set mode to 'production'
3. set mode to 'none'

I think I prefer option 1, since some of the development features enabled by webpack are useful for test runners too:

• devtool: 'eval'
• in-memory caching (helps with rebuilds in watch mode)
• namedModules
• namedChunks
• NODE_ENV DefinePlugin (mode: 'none' currently results in NODE_ENV of 'production' which will be changed in webpack 5 to be undefined instead)

F) --mode {production,development} and NODE_ENV having a matching value:

--> Use the provided values for both, since they are already set and consistent with each other.

G) Both --mode and NODE_ENV set, but with different values:

1. Have --mode take priority, and so ignore the original value for NODE_ENV, and so falling back to scenario A or B above.
2. Use the provided values for both as-is.

The argument for option 1 would be that generally CLI options take precedence over environment variables (and in some cases users might not realise NODE_ENV is set for them, eg Heroku builds, so get confused when --mode development doesn't work). And the argument for option 2 would be that it would allow doing --mode {development,production} with NODE_ENV=test -- though that's already possible if we pick a suitable option for scenario E above.

As such I think I'm leaning towards option 1.

---

@eliperelman, what are your thoughts?

[eliperelman]

@edmorley reading through all of that, I think I am mostly in agreement.

For option C, I think throwing is acceptable since webpack technically also is supposed to throw if no mode is set.

Are there any options where you feel torn and you'd like me to weigh my opinion to tie-break?

[edmorley]

Scenario B is one I'm not sure about what we should do.

[edmorley]

G) Both --mode and NODE_ENV set, but with different values:

• Have --mode take priority, and so ignore the original value for NODE_ENV, and so falling back to scenario A or B above.
• Use the provided values for both as-is.

The argument for option 1 would be that generally CLI options take precedence over environment variables (and in some cases users might not realise NODE_ENV is set for them, eg Heroku builds, so get confused when --mode development doesn't work). And the argument for option 2 would be that it would allow doing --mode {development,production} with NODE_ENV=test -- though that's already possible if we pick a suitable option for scenario E above.

As such I think I'm leaning towards option 1.

Another argument for (1), is that webpack uses mode to determine the NODE_ENV value passed to DefinePlugin, rather than NODE_ENV itself.

As such, if we went with (2), using NODE_ENV=development webpack --mode production would mean there were actually two values for NODE_ENV -- "development" in the Neutrino configs, and "production" when referenced in the built source itself. See:
https://github.com/webpack/webpack/blob/v4.14.0/lib/WebpackOptionsDefaulter.js#L302-L305
https://github.com/webpack/webpack/blob/v4.14.0/lib/WebpackOptionsApply.js#L364-L368