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

🐛 Fix environemnt specific build configuration #37

Merged
merged 3 commits into from
Mar 20, 2021

Conversation

jstorm31
Copy link

CRA's --target parameter only supports production value, not development, which results in console error with production build. As discussed here, custom env variable should be used for that. In our case REACT_APP_BUILD_ENV.

I also fixed build commands naming so they correspond to the current Gitlba CI configuration.

@jstorm31 jstorm31 requested a review from a team March 11, 2021 14:46
@jstorm31 jstorm31 self-assigned this Mar 11, 2021
@jstorm31
Copy link
Author

I forgot to update config.js and env.js and set corrrect env variable there.

@cermakjiri
Copy link
Member

cermakjiri commented Mar 12, 2021

Based on the --target param. NODE_ENV and BUILD_ENV were automatically set. If we're going to define REACT_APP_BUILD_ENV before each build script, I'd suggest defining NODE_ENV before each build script as well and removing the --target completely. And so the packages/react-scripts/custom/config/env.js file can be then removed.

@jstorm31
Copy link
Author

jstorm31 commented Mar 13, 2021

@cermakjiri Thank you for the feedback.

From the documentation of CRA:

There is also a built-in environment variable called NODE_ENV. You can read it from process.env.NODE_ENV. When you run npm start, it is always equal to 'development', when you run npm test it is always equal to 'test', and when you run npm run build to make a production bundle, it is always equal to 'production'. You cannot override NODE_ENV manually. This prevents developers from accidentally deploying a slow development build to production.

So the --target param shouldn't affect it. Do you have any resource on that? Anyway it doesn't work for sure with --target stage as stated in the issue linked in the above description. There is even an error about production build in console when --target development is used.

So my conclusion to resolve the issue:

  1. Is BUILD_ENV used for anything, how is it set and do we further need it?
  2. We shouldn't set NODE_ENV manually, because it has always production value for yarn build and development for yarn start`.
  3. We should remove --target as it clearly doesn't affect anything.
  4. I don't agree with removing config/env.js. We define all environments used in code there and further export variables like isEnvDevelopment, etc. for convenience rather then always using process.env.REACT_APP_BUILD_ENV.

@cermakjiri
Copy link
Member

@cermakjiri Thank you for the feedback.

From the documentation of CRA:

There is also a built-in environment variable called NODE_ENV. You can read it from process.env.NODE_ENV. When you run npm start, it is always equal to 'development', when you run npm test it is always equal to 'test', and when you run npm run build to make a production bundle, it is always equal to 'production'. You cannot override NODE_ENV manually. This prevents developers from accidentally deploying a slow development build to production.

So the --target param shouldn't affect it. Do you have any resource on that? Anyway it doesn't work for sure with --target stage as stated in the issue linked in the above description. There is even an error about production build in console when --target development is used.

So my conclusion to resolve the issue:

  1. Is BUILD_ENV used for anything, how is it set and do we further need it?
  2. We shouldn't set NODE_ENV manually, because it has always production value for yarn build and development for yarn start`.
  3. We should remove --target as it clearly doesn't affect anything.
  4. I don't agree with removing config/env.js. We define all environments used in code there and further export variables like isEnvDevelopment, etc. for convenience rather then always using process.env.REACT_APP_BUILD_ENV.
  1. Only here - packages/react-scripts/custom/config/transformWebpackConfig.js and it just needs to be replaced with process.env.REACT_APP_BUILD_ENV.
  2. Yes, make sense.
  3. Yes, I agree. It was created by me but it has turned out to not be as a good feature as I initially thought it will 😄
  4. Well the packages/react-scripts/custom/config/env.js was created by me and it can be safely removed. Don't misplace it with config/env.js, we definitely do not wanna remove that.

@jstorm31
Copy link
Author

Good, I updated packages/react-scripts/custom/config/env.js and packages/react-scripts/custom/config/transformWebpackConfig.js to use REACT_APP_BUILD_ENV and removed --target flag.

@jstorm31 jstorm31 merged commit d451ceb into master Mar 20, 2021
@jstorm31 jstorm31 deleted the fix/env-specific-build branch March 20, 2021 09:57
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

4 participants