-
Notifications
You must be signed in to change notification settings - Fork 73
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
REMOVE nodeenv development #81
Conversation
What about the |
The commit where it came from is super old: https://github.com/chromaui/chromatic/commit/e1a9ba09283ff96a6c9b369e855db5c3bd41efba |
The NODE_ENV=TEST was to make babel parse the example storybook correctly 😖 Removing it caused the example to fail on require.context I was able to make the example work by removing the babel stuff for webpack's require.context, and migrating to tri-config. |
I merged in the better error handling, because it actually helped my find & test the problem. I could remove it from this branch again if you really want @tmeasday or I could close the other PR, and consider it 1 cleanup PR. WDYT? |
Hold on a sec, you might need to talk me through what happened here. Definitely pull the other PR out as I had comments on that separately. |
b1cdcdd
to
fb0f739
Compare
I cleaned the PR up @tmeasday |
Thanks it looks reasonable. I'm just worried that you had to do this. Does it mean that Chromatic doesn't work any more with non- |
NO AFAICS it should work without / should work with the old config.js just fine @tmeasday |
So why did you need to do it in our test storybook? Can you explain what happened? Is the |
@tmeasday build-storybook uses the babel config defined in the root. It's always done that, but because NODE_END was set to 'development' and in the root there's no babel config for that env, it wouldn't override anything. Removing NODE_ENV='development' made it act as NODE_ENV='test' which DOES match a env specified in the root babel config, and thus it's applied. Turns out if we add Line 3 in fb07c93
IMHO that should be |
We can't know exactly what impact a NODE_ENV change will have on the users config/code-output. Perhaps it's safer to release this as a major? |
AFACS there's no usage of process.env.BROWSER whatsoever. Removing seems to do absolutely nothing. |
OK, setting
You are probably right. |
I'll delete this code and that makes this a major release
|
# Conflicts: # package.json # yarn.lock
…chromatic-cli into fix/remove-nodeenv-development
fixes: #68