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

Make browsers list configurable #2358

Closed
wants to merge 25 commits into from
Closed

Make browsers list configurable #2358

wants to merge 25 commits into from

Conversation

cr101
Copy link
Contributor

@cr101 cr101 commented May 25, 2017

Autoprefixer's browsers option is currently hardcoded. This PR adds the browserslist key in package.json which allows developers to set different browsers for both production and development.
Autoprefixer which relies on Browserslist will find its config automatically.
This PR fixes #2248 and #892

cr101 added 2 commits May 25, 2017 15:58
…he browserslist key in package.json which allows developers to set browsers list in queries like "Chrome >= 35", "Firefox >= 38" for both production and development. Autoprefixer which relies on Browserslist will find its config automatically. This PR fixes facebook#2248 and facebook#892
@gaearon
Copy link
Contributor

gaearon commented May 25, 2017

Does this let me specify browserslist in package.json of unejected apps? We should support that (with our hidden default if it’s unspecified).

@gaearon gaearon added this to the 2.0.0 milestone May 25, 2017
@cr101
Copy link
Contributor Author

cr101 commented May 25, 2017

@gaearon

Does this let me specify browserslist in package.json of unejected apps?

Yes. Running create-react-app my-app will add the default browserslist in package.json as below:

screenshot_6

@gaearon
Copy link
Contributor

gaearon commented May 26, 2017

Do you think it would make sense to use a single list by default, and document splitting dev/prod in user guide? Not sure if it would be confusing to most people or not.

I also think we should still provide a default for people who don't have a browserslist in their package.json. Since they probably will forget to add it upgrading. We should either force it to be there, or provide a good default.

@cr101
Copy link
Contributor Author

cr101 commented May 30, 2017

@gaearon I agree that if people forget to add browserslist to their package.json when they upgrade then we should force it to be there. If you agree with the approach then I will do the user documentation next.

Do you think it would make sense to use a single list by default, and document splitting dev/prod in user guide? Not sure if it would be confusing to most people or not.

A number of people have asked to split dev/prod

@gaearon
Copy link
Contributor

gaearon commented May 30, 2017

I'd like to split these changes if you don't mind.

Let's start with using default values when browserslist is not specified. We can force it to be specified in a later release, but I'd rather release this as a non-breaking change for now.

I would also like to support both single array and an object with development and production configurations.

let supportedBrowsers = require(paths.appPackageJson).browserslist;
if (!supportedBrowsers || Object.keys(supportedBrowsers).length === 0) {
console.log(
chalk.yellow(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to not introduce warnings for this.
It will be a straightforward step in migration guide, but for now I don't want people to get annoyed.

@cr101
Copy link
Contributor Author

cr101 commented Jun 29, 2017

Sorry, I accidentally messed up committing my changes while merging changes from upstream.

@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2017

Hmm. I wonder if we should count this as a breaking change or not. I guess not.

@gaearon gaearon modified the milestones: 1.x, 2.0.0 Jun 29, 2017
@Timer
Copy link
Contributor

Timer commented Jun 29, 2017

IIRC, browserlist searches all the way up the folder tree. This might cause confusing situations if users have dangling package.jsons (something we've seen before) or malformed ones (it complains loudly when malformed).
Can we limit its search scope?

@cr101
Copy link
Contributor Author

cr101 commented Jul 2, 2017

@Timer Any idea how to limit the browserlist search scope?

@@ -23,6 +23,14 @@ const ModuleScopePlugin = require('react-dev-utils/ModuleScopePlugin');
const paths = require('./paths');
const getClientEnvironment = require('./env');

// Get supported browsers list
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be moved to a separate module to avoid DRY, maybe config/getSupportedBrowsers?

@Timer
Copy link
Contributor

Timer commented Jul 2, 2017

@cr101 https://github.com/ai/browserslist/blob/master/index.js#L361 looks like it can't be configured; open an issue and PR maybe?

@cr101
Copy link
Contributor Author

cr101 commented Jul 2, 2017

@Timer browserslist just does a JSON.parse of package.json to find the list of browsers section.

@Timer
Copy link
Contributor

Timer commented Aug 3, 2017

What needs done here?
Have we made that pull request to limit the scope searching?

@cr101
Copy link
Contributor Author

cr101 commented Aug 3, 2017

Have we made that pull request to limit the scope searching?

I don't believe so. I'm now on Windows which makes it tricky to test the changes.
@Timer I'd appreciate your help with limiting the search scope.

@gaearon
Copy link
Contributor

gaearon commented Jan 14, 2018

Really sorry about this—we were very busy with React 16 rollout and by the time we got around to reviewing #3644 was more fresh so we took it instead. Thanks for prototyping this early on!

@gaearon gaearon closed this Jan 14, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 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.

Option for enabling Flexbox 2009
5 participants