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

Resolve #1085: Free __DEV__ variable provided by webpack should be used across RSK i… #1088

Closed
wants to merge 10 commits into from

Conversation

buildbreakdo
Copy link

@buildbreakdo buildbreakdo commented Jan 14, 2017

Resolve #1085
To my knowledge, __DEV__ is a facebook code idiom that showed up relatively recently in the main React repo. Can see it all over the place here (may need to login to see github search results). Their build process has a step that replaces __DEV__ with process.env.NODE_ENV !== 'production'.

RSK currently has something similar, __DEV__ is made available via webpack, we just have not been using it. This is likely due to a missing exception in the .eslintrc allowing a no-underscore-dangle exception for __DEV__ (and lack awareness of availability too!).

Can see where __DEV__ is made available in the current webpack.config.js here (same for client/server):

// tools/webpack.config.js
... 
   // Define free variables
    // https://webpack.github.io/docs/list-of-plugins.html#defineplugin
    new webpack.DefinePlugin({
      'process.env.NODE_ENV': isDebug ? '"development"' : '"production"',
      'process.env.BROWSER': true,
      __DEV__: isDebug,
    }),
...

Pull request would replace both process.env.NODE_ENV !== 'production' statements with __DEV__ and add a no-comma-dangle exception to the .eslintrc. isDebug in webpack.config.js should also be converted into __DEV__ as there is no reason to violate convention.

++Readability

(p.s. Sorry for the pull request spam.. eslint --fix which runs precommit alters code WHILE committing. So __DEV__ = true if consumed like {option1: 'foo', __DEV__ : __DEV__} is transformed and committed as {option1: 'foo', __DEV__} which is less readable and certainly not a norm.. have never seen this pattern. Honestly did not even know var x = true; var y = {x}; was valid JS. Completely unexpected behaviour. Ended up disabling es-lint for those lines)

'process.env.BROWSER': true,
__DEV__: isDebug,
__DEV__: __DEV__, // eslint-disable-line
Copy link
Collaborator

Choose a reason for hiding this comment

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

-      __DEV__: __DEV__, // eslint-disable-line
+      __DEV__,

@buildbreakdo
Copy link
Author

@langpavel Thanks for the review, merged with latest webpack v2.2.1 changes and removed the eslint-disable-line. Cheers

@buildbreakdo
Copy link
Author

buildbreakdo commented Feb 2, 2017

@langpavel Forgot to remove the same line from the server config. Done now 👍

@langpavel
Copy link
Collaborator

Rebased in #1114.

@langpavel langpavel closed this Feb 2, 2017
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.

Free __DEV__ variable provided by webpack should be used instead of NODE_ENV checks
8 participants