don't clobber user-defined environment variables #7878

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@vkarpov15
Contributor

vkarpov15 commented Jun 1, 2016

If I want to set NODE_ENV to "baconator", I should be allowed to. Mutating global state that most devs assume to be immutable is just abysmal dev practice, especially since this mutation only happens when you're building for prod, not running on the simulator.

To test this, run env NODE_ENV=baconator ./gradlew assembleRelease with babel-plugin-transform-inline-environment-variables in your app/.babelrc. You'll see that the final app has NODE_ENV=production.

As a side note, running with babel-plugin-transform-inline-environment-variables in the top-level .babelrc crashes horribly with a compiler error.

For anybody who runs into this bug and doesn't feel like waiting for this to get merged, I wrote a quick babel plugin to remove assignments to process.env, which is sufficient to fix this issue.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 1, 2016

By analyzing the blame information on this pull request, we identified @sam-swarr and @davidaurelio to be potential reviewers.

ghost commented Jun 1, 2016

By analyzing the blame information on this pull request, we identified @sam-swarr and @davidaurelio to be potential reviewers.

@javache

This comment has been minimized.

Show comment
Hide comment
@javache

javache Jun 3, 2016

Member

We need this to make sure React runs with the right flags. If we didn't have this, you'd be running all sort of potentially expensive checks in production!

How about making this first check if NODE_ENV is undefined and only doing our args.dev change if it hasn't been set yet.

Member

javache commented Jun 3, 2016

We need this to make sure React runs with the right flags. If we didn't have this, you'd be running all sort of potentially expensive checks in production!

How about making this first check if NODE_ENV is undefined and only doing our args.dev change if it hasn't been set yet.

@alexprice1

This comment has been minimized.

Show comment
Hide comment
@alexprice1

alexprice1 Jun 10, 2016

@javache +1 on the only setting it if NODE_ENV is undefined. If a put in a PR, will you accept?

@javache +1 on the only setting it if NODE_ENV is undefined. If a put in a PR, will you accept?

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Jun 10, 2016

Contributor

Closed in favor of #8057

Contributor

vkarpov15 commented Jun 10, 2016

Closed in favor of #8057

@vkarpov15 vkarpov15 closed this Jun 10, 2016

ghost pushed a commit that referenced this pull request Jun 10, 2016

Don't clobber user-defined environment variables
Summary:
Re: javache 's suggestions from #7878. Didn't want to deal with the merge conflict so I'm opening a separate PR. Here's the original justification:

If I want to set NODE_ENV to "baconator", I should be allowed to. Mutating global state that most devs assume to be immutable is just abysmal dev practice, especially since this mutation only happens when you're building for prod, not running on the simulator.

To test this, run env NODE_ENV=baconator ./gradlew assembleRelease with babel-plugin-transform-inline-environment-variables in your app/.babelrc. You'll see that the final app has NODE_ENV=production.

As a side note, running with babel-plugin-transform-inline-environment-variables in the top-level .babelrc crashes horribly with a compiler error.

For anybody who runs into this bug and doesn't feel like waiting for this to get merged, I wrote a quick babel plugin to remove assignments to process.env, which is sufficient to fix this issue.
Closes #8057

Differential Revision: D3419950

Pulled By: javache

fbshipit-source-id: dc541cad0a99906433e5c14bbc93ce66b4ed325e

samerce added a commit to iodine/react-native that referenced this pull request Aug 23, 2016

Don't clobber user-defined environment variables
Summary:
Re: javache 's suggestions from facebook#7878. Didn't want to deal with the merge conflict so I'm opening a separate PR. Here's the original justification:

If I want to set NODE_ENV to "baconator", I should be allowed to. Mutating global state that most devs assume to be immutable is just abysmal dev practice, especially since this mutation only happens when you're building for prod, not running on the simulator.

To test this, run env NODE_ENV=baconator ./gradlew assembleRelease with babel-plugin-transform-inline-environment-variables in your app/.babelrc. You'll see that the final app has NODE_ENV=production.

As a side note, running with babel-plugin-transform-inline-environment-variables in the top-level .babelrc crashes horribly with a compiler error.

For anybody who runs into this bug and doesn't feel like waiting for this to get merged, I wrote a quick babel plugin to remove assignments to process.env, which is sufficient to fix this issue.
Closes facebook#8057

Differential Revision: D3419950

Pulled By: javache

fbshipit-source-id: dc541cad0a99906433e5c14bbc93ce66b4ed325e

mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016

Don't clobber user-defined environment variables
Summary:
Re: javache 's suggestions from facebook#7878. Didn't want to deal with the merge conflict so I'm opening a separate PR. Here's the original justification:

If I want to set NODE_ENV to "baconator", I should be allowed to. Mutating global state that most devs assume to be immutable is just abysmal dev practice, especially since this mutation only happens when you're building for prod, not running on the simulator.

To test this, run env NODE_ENV=baconator ./gradlew assembleRelease with babel-plugin-transform-inline-environment-variables in your app/.babelrc. You'll see that the final app has NODE_ENV=production.

As a side note, running with babel-plugin-transform-inline-environment-variables in the top-level .babelrc crashes horribly with a compiler error.

For anybody who runs into this bug and doesn't feel like waiting for this to get merged, I wrote a quick babel plugin to remove assignments to process.env, which is sufficient to fix this issue.
Closes facebook#8057

Differential Revision: D3419950

Pulled By: javache

fbshipit-source-id: dc541cad0a99906433e5c14bbc93ce66b4ed325e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment