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

Clarifying name and use of React env config. #184

Merged
merged 1 commit into from Jul 8, 2016
Merged

Clarifying name and use of React env config. #184

merged 1 commit into from Jul 8, 2016

Conversation

HoraceShmorace
Copy link
Contributor

  1. "GLOBALS" is ambiguous, particularly since its only use is to configure the React environment. I don't really see a reason to abstract this at all, as opposed to just inlining the object in the call to webpack.DefinePlugin.
  2. Clarified the comment on line 25 to indicate that the statement before it sets the build to either mode, not just prod.

Note: There should probably be a comment on line 6 to explain the need to JSON.stringify a string. I'm not sure of the reason, so I couldn't add the comment myself.

1. "GLOBALS" is ambiguous, particularly since its only use is to configure the React environment. I don't really see a reason to abstract this at all, as opposed to just inlining the object in the call to `webpack.DefinePlugin`.

2. Clarified the comment on line 25 to indicate that the statement before it sets the build to either mode, not just prod.

Note: There should probably be a comment on line 6 to explain the need to `JSON.stringify` a string. I'm not sure of the reason, so I couldn't add the comment myself.
@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage remained the same at 98.906% when pulling a9be5dd on HoraceShmorace:master into a7c95c5 on coryhouse:master.

@coryhouse coryhouse merged commit eeabfbc into coryhouse:master Jul 8, 2016
@coryhouse
Copy link
Owner

Thanks for the PR!

@coryhouse
Copy link
Owner

And I just inlined the code as you suggested. Thanks!

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.

None yet

3 participants