webhintio / rfcs

🛑 This repository is deprecated!! To request changes, new features, hints, etc. please open an issue in https://github.com/webhintio/hint
1 stars 1 forks source link

Support for .browserlistrc #59

Closed CKGrafico closed 5 years ago

CKGrafico commented 5 years ago

At this moment, webhint is supporting the browserlist configured in the .hintrc file and inside package.json file. But in some project users have defined the browserlist in .browserlistrc

To do that will also be necessary to "merge" the list of borwsers (or similar solution) if the user uses .browserlistrc + .hintrc.

CC/ @molant

molant commented 5 years ago

We should add support for this but I'm concerned about what the right behavior should be. The user will be able to define a browserslist from different places: .hintrc,.browserslistrc, andpackage.json` and this can cause issues when the contents are different:

IMO .hintrc should take precedence (I think that's what we are doing right now between hintrc and package.json) and maybe we should print a warning saying that there are differences between the different browserslist configuration. I could see someone having a .browserslistrc for autoprefixer that's going to generate CSS for older browsers but still use progressive enhancement for some of them, and another set of browsers in .hintrc for polyfilling JS features.

molant commented 5 years ago

@webhintio/contributors what do you think?

antross commented 5 years ago

Seems like we should let browserslist do it's thing if there isn't a list defined in .hintrc. Per the JS API documentation for browserslist:

If a query is missing, Browserslist will look for a config file. You can provide a path option (that can be a file) to find the config file relatively to it.

So as long as we don't explicitly pass a query (which we should only do if we have a list specified in .hintrc) then we should get something along the lines of precedence you describe. Also, if we follow this approach I don't think a warning is necessary from webhint as IMO we're better off staying relatively ignorant about where else browserslist might get it's configuration from.

Additionally we should update our documentation to recommend users configure browserslist via package.json as that's the best place to avoid differing definitions and it's what browserslist itself recommends:

Browserslist will use browsers and Node.js versions query from one of this sources:

  1. browserslist key in package.json file in current or parent directories. We recommend this way.
  2. ...
antross commented 5 years ago

In short, maybe we just need a fix to do a better job of "getting out of the way".

isabellachen commented 5 years ago

If the project has browserslist defined in .hintrc and package.json, the definition in .hintrc takes precedence. Browserslist looks for its key in package.json by default so looks like the current code to grab the Browserslist config from package.json if no definition was found in .hintrc is not necessary since we are duplicating work done by Browserslist's default behaviour.

Browserlists throws an error if users define the property in both package.json and .browserlistrc. Perhaps we could follow this and throw an error also if a user has defined a .browserslist config in both .hintrc and package.json. As a user, had I defined the config in both package.json and .hintrc, I would be confused as to whether the configs were merged or if one had taken precedence over another.

Alternatively, we can leave the current situation (.hintrc definition takes precedence over package.json definition) and document it in the Browser Configuration page so the user is aware of the hierarchy and does not expect any merging to happen. In this case, the only improvement to the code I can see is to remove the lines do the duplicate work of looking in package.json - that and adding some tests...

@antross @borgitas21

antross commented 5 years ago

Browserlists throws an error if users define the property in both package.json and .browserlistrc. Perhaps we could follow this and throw an error also if a user has defined a .browserslist config in both .hintrc and package.json. As a user, had I defined the config in both package.json and .hintrc, I would be confused as to whether the configs were merged or if one had taken precedence over another.

@isabellachen I like this suggestion. I think having explicit errors is the cleanest approach to help avoid confusion and I can't think of a compelling use-case for configuring browsers differently for webhint than the rest of of a project. Let's go this route, including removing the duplicate work of looking at package.json you pointed out.

antross commented 5 years ago

Update from webhintio/hint#1599:

After digging into this further, I also confirmed that .browserslistrc already works with the current version of webhint (perhaps the original bug was a misunderstanding of our documentation). Thus, the only additional functionality we might add from the discussion in webhintio/rfcs#59 would be throwing an error if a user specifies a browserslist in more than one configuration file. I don't think that's high enough priority to add on it's own.

Rather than refactor this code to avoid the dead APIs just to introduce an error message, I suggest we close this PR and move on to a different task.

antross commented 5 years ago

Considering there isn't a functional bug here and we're now tracking clarifying the documentation in webhintio/hint#1611, I'm closing this issue.