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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define NODE_ENV on client-side in real world example #1605

Closed
wants to merge 1 commit into from

Conversation

JeremyBernier
Copy link

Right now NODE_ENV is always undefined on the client-side. This fix allows NODE_ENV to be defined, thus giving us the option to run the app in production mode (currently it'll only run in dev mode - see configureStore.js).

Right now NODE_ENV is always undefined on the client-side. This fix allows NODE_ENV to be defined, thus giving us the option to run the app in production mode (currently it'll only run in dev mode - see configureStore.js).
new webpack.HotModuleReplacementPlugin(),
new webpack.DefinePlugin({
'process.env': {
NODE_ENV: JSON.stringify(process.env.NODE_ENV)
Copy link
Contributor

Choose a reason for hiding this comment

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

Afaik this won't work since we don't define the NODE_ENV in the start command, so process.env.NODE_ENV will be undefined.

Copy link

Choose a reason for hiding this comment

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

We could do this, then:

NODE_ENV: JSON.stringify(process.env.NODE_ENV || 'development')

Copy link
Contributor

Choose a reason for hiding this comment

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

That won't work either, it'll never be in production. We don't even have a script to run this in production, see the package.json: https://github.com/JeremyBernier/redux/blob/patch-1/examples/real-world/package.json

Copy link

Choose a reason for hiding this comment

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

👍

Copy link
Author

Choose a reason for hiding this comment

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

Yea it would be undefined if you ran npm start, but not if you ran NODE_ENV=production npm start

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2016

These examples are meant as Redux examples, not as boilerplates you can run and bundle completely. I’d welcome an effort adding an npm run build script to each of them but just defining NODE_ENV is not enough: you also need to remove hot module replacement stuff, enable Uglify, etc.

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2016

I’ll close this specific PR but please feel free to submit another one that converts all examples to have a proper build.

@gaearon gaearon closed this Apr 13, 2016
@JeremyBernier
Copy link
Author

Yea it's definitely not a complete solution, but I figured it would only help to add since the example currently checks for process.env.NODE_ENV on the client-side (in configureStore.js), so one would inevitably have to add this line of code for that variable to ever not be undefined on the client-side. But I totally understand if you'd prefer to only add in a full working solution.

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2016

so one would inevitably have to add this line of code for that variable to ever not be undefined on the client-side

Not quite sure what you mean. It’s fine for it to be undefined in dev. And the examples are not production-ready anyway so just fixing this one issue might actually make it worse because it’ll give the impression that they are production-ready whereas in reality there is more work to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants