fix(env): do not override NODE_ENV if already set

[aversini]

This recent styleguidist update to support Create React App is overriding NODE_ENV, even if it's already set by the user.

This PR is an attempt to fix this and still support Create React App:

• If NODE_ENV is set, use it
• In ReactComponent, instead of checking for development, check for not production, allowing the use of debug for example and treat it the same as development

This is allowing a user to do this:

NODE_ENV=debug styleguidist server

and configure their webpack/styleguidist configuration to act accordingly on 'debug'

[sapegin]

Cool, thanks! Could you please check CI?

[sapegin]

Good catch!

[aversini]

Actually that is what causing CI to fail.

I'm assuming Jest snapshots have been taken with NODE_ENV set to 'production'. In CI, the test runs with NODE_ENV=test, so it makes my test (process.env.NODE_ENV !== 'production') truthy. That means that in test mode, component are not rendered in 'production' mode, and therefore the snapshots do not match.

Before my change, with NODE_ENV set to 'test' would render the component in production mode... quite a conundrum here. I have a feeling that it would make more sense to revert this change and keep snapshots 'production'. It means that ONLY when specifically set to 'development', component are rendered that special way? Let me know what you think @sapegin

[sapegin]

I thinks != production is more common way to check that but we’ll probably need to grep the code for development and update checks in other places too ;-)

[aversini]

You're right. So I grep'd around and didn't find any places that looked like they should be updated, beside that one line. So my suggestion is to consider the 'test' path by updating the condition to:

const ExamplePlaceholder = process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test'
  ? require('rsg-components/ExamplePlaceholder').default
  : () => <div />;

This has no impact for production code and it should allow CI to pass.

[sapegin]

I think we can just update snapshots because it doesn’t really matter if it renders placeholder or not if it does the right thing ;-)

[codecov-io]

Codecov Report

Merging #474 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #474   +/-   ##
=======================================
  Coverage   95.23%   95.23%           
=======================================
  Files          98       98           
  Lines        1280     1280           
  Branches      264      264           
=======================================
  Hits         1219     1219           
  Misses         59       59           
  Partials        2        2

Impacted Files	Coverage Δ
...rc/rsg-components/ReactComponent/ReactComponent.js	83.33% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b1be52...9055d4b. Read the comment docs.

[aversini]

@sapegin I finally got around this PR and by virtue of synchronizing it with the latest from master, CI tests are all passing 😄 Let me know if you need anything else. Thanks!

[sapegin]

Awesome, thanks!