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

Allow for browserslist environment variables #7764

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

GreenGremlin
Copy link
Contributor

@GreenGremlin GreenGremlin commented Oct 1, 2019

Browserslist allows for setting browserslist config using environment variables, but CRA still prompts for setting browserslist config when it is already configured for the environment.

For example, running this currently results in CRA improperly detecting that browserslist is not configured, even though it is.

BROWSERSLIST="> 5%" npm run build

@GreenGremlin
Copy link
Contributor Author

It looks like the build is now passing for other MRs, can someone please re-run CI on this MR?

@GreenGremlin
Copy link
Contributor Author

It looks like the build is now passing for other MRs, can someone please re-run CI on this MR?

Never mind, I just pushed an amended commit.

@mrmckeb
Copy link
Contributor

mrmckeb commented Oct 9, 2019

@GreenGremlin We've had some problems with out CR, sorry that you had to push to get your PR (😉) to pass.

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, can you share the documentation on this? I couldn't find it on their repo.

@GreenGremlin
Copy link
Contributor Author

This looks good, can you share the documentation on this? I couldn't find it on their repo.

Unfortunately, just like findConfig there doesn't appear to be documentation on loadConfig. You can see from the loadConfig implementation that, unlike findConfig which just returns the path to the config, it returns the actual config. The primary difference here being that it also checks process.env.BROWSERSLIST and process.env.BROWSERSLIST_CONFIG, which findConfig doesn't account for.

@mrmckeb mrmckeb added this to the 3.3 milestone Oct 9, 2019
Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look there, thanks! I've tagged this for the next minor.

@ianschmitz
Copy link
Contributor

It's interesting that we're setting this in our e2e tests. I imagine it must not be doing anything?

export BROWSERSLIST='ie 9'

@GreenGremlin
Copy link
Contributor Author

It's interesting that we're setting this in our e2e tests. I imagine it must not be doing anything?

export BROWSERSLIST='ie 9'

The environment variables work for everything but the check for browserslist config that determines wether we should prompt to add it. Also, if the e2e tests are not run interactively, we may just skip the prompt entirely.

@ianschmitz ianschmitz modified the milestones: 3.3, 3.2.1 Oct 11, 2019
@ianschmitz ianschmitz merged commit f06ae8b into facebook:master Oct 11, 2019
@ianschmitz
Copy link
Contributor

Thanks @GreenGremlin!

@lock lock bot locked and limited conversation to collaborators Oct 16, 2019
@iansu iansu modified the milestones: 3.2.1, 3.3 Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants