set NODE_ENV to production

[CallumVass]

Fix #392

[theimowski]

If this is "best practice" I'm wondering whether we should force it for minimal template - maybe just use the best practice for default template and let minimal users handle that themselves if necessary?

[CallumVass]

I don't mind, I was just being consistent in applying it to both versions of the template as they both issue the same command but I can see your point. I'm happy to take it out of the minimal template? Let me know.

[theimowski]

yes please - let's keep the minimal minimal :)

[CallumVass]

Hi, that's done. Thanks

[kulshekhar]

Doesn't webpack -p automatically set this?

Running webpack --help shows:

  -p           shortcut for --optimize-minimize --define process.env.NODE_ENV="production"

[olivercoad]

@kulshekhar Only in the bundle for runtime, it doesn't set it for build time.

@CallumVass is there any reason not to just override it in webpack.config.js?

process.env.NODE_ENV = isProduction ? "production" : ""

Or how about setting isProduction = process.env.NODE_ENV === 'production' instead?

I can just envision creating problems from having two sources of truth for the mode used in different places.

[kulshekhar]

The following statement from the webpack docs make me think -p would be sufficient. I'm no webpack expert though so if you're sure, please ignore my post

If you're using a library like react, you should actually see a significant drop in bundle size after adding DefinePlugin

In this case, adding DefinePlugin and using the -p flag are equivalent.

[olivercoad]

Just above that it says

Contrary to expectations, process.env.NODE_ENV is not set to 'production' within the build script

[kulshekhar]

My understanding of that statement (based on what follows) is that process.env.NODE_ENV won't be set to production as far as the code within the webpack configuration file is concerned but will build everything in production mode. In other words, if you have code in the webpack file that depends on NODE_ENV being set to production then you'd need to set the env variable when invoking webpack.

Edit: a quick glance at https://github.com/SAFE-Stack/SAFE-template/blob/master/Content/webpack.default.config.js#L52 indicates that the config file itself doesn't need the NODE_ENV variable and uses a different mechanishm to determine whether it should be building for production or not

[CallumVass]

Hi,

Setting -p flag when running webpack doesn't set NODE_ENV to production from what I saw with my issue. If you wanted me to put up a sample repo then I am more then happy to. I'm also not a front-end expert so setting it as part of the npm build script seemed the best place to me? @Zaid-Ajaj recommended it here too: #392 (comment)

[kulshekhar]

is there any reason not to just override it in webpack.config.js?

process.env.NODE_ENV = isProduction ? "production" : ""

I can just envision creating problems from having two sources of truth for the mode used in different places.

This seems like a good solution which

• won't require new dependencies
• will have one source of truth

If an externally provided env variable is needed anyway, it might be better to use the --env flag that webpack has. That doesn't set NODE_ENV but the value from there can be used to set both isProduction and NODE_ENV somewhere at the top of the script.

[CallumVass]

Hi,

I've set NODE_ENV inside the webpack.config now. Removed the cross-env dependency and reverted all my other changes.

[isaacabraham]

LGTM

[theimowski]

Please also apply same changes to Content/webpack.tests.config.js to keep consistent

[CallumVass]

Please also apply same changes to Content/webpack.tests.config.js to keep consistent

That's done. Thanks for reviewing.

[theimowski]

thanks!