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

Issues/browserslist config #1599

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@borgitas21
Copy link
Contributor

borgitas21 commented Jan 2, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

@borgitas21 borgitas21 requested review from alrra , antross , molant and sarvaje as code owners Jan 2, 2019

@isabellachen

This comment has been minimized.

Copy link
Contributor

isabellachen commented Jan 2, 2019

This is ready for review @molant @antross @sarvaje @alrra

isabellachen added some commits Jan 2, 2019

Show resolved Hide resolved packages/hint/src/lib/config.ts Outdated
Show resolved Hide resolved packages/hint/src/lib/types/config-basename.ts Outdated
Show resolved Hide resolved packages/hint/src/lib/config.ts Outdated
Show resolved Hide resolved packages/hint/src/lib/config.ts Outdated
Show resolved Hide resolved packages/hint/src/lib/types.ts Outdated
Show resolved Hide resolved ...int/tests/lib/fixtures/browserslist-package-json-hintconfig/package.json Outdated
Show resolved Hide resolved ...es/hint/tests/lib/fixtures/browserslist-package-json-hintrc/package.json Outdated
Show resolved Hide resolved ...ges/hint/tests/lib/fixtures/browserslist-valid-package-json/package.json Outdated

if (basename === ConfigFile.PackageJson) {

const packageJson = loadJSONFile(file);

This comment has been minimized.

@antross

antross Jan 2, 2019

Member

Since Configuration.loadConfigFile already handles loading package.json I'm confused as to why this separate load is needed. Is there a particular reason this needs to be special-cased that I'm missing?

This comment has been minimized.

@isabellachen

isabellachen Jan 3, 2019

Contributor

The way I understand it, it's possible to declare browserslist in both the root of the package.json and in the hintConfig prop in the package.json. So I'm trying to catch the case if browserslist has been declared twice in package.json, once in the root and once in the hintConfig prop.

This comment has been minimized.

@antross

antross Jan 3, 2019

Member

Ah, now it makes sense :) Thanks for clarifying!

Let's add comments to the individual checks explaning which scenarios each is handling. Also, I might rename the function groupBrowsersListConfigs or similar since browserslist is the focus here.

}

if (hintConfig && hintConfig.browserslist) {
configs.hintConfig = hintConfig.browserslist;

This comment has been minimized.

@antross

antross Jan 2, 2019

Member

What does this do?

This comment has been minimized.

@isabellachen

isabellachen Jan 3, 2019

Contributor

It gets the value in the user's hintConfig property declared in the package.json file.

This comment has been minimized.

@isabellachen

isabellachen Jan 3, 2019

Contributor

Since loadConfigFile() gets the hintConfig value and loadJsonFile gets the contents of the entire package.json.

@alrra alrra force-pushed the webhintio:master branch from 6a9ac06 to 9b2a76e Jan 2, 2019

@antross
Copy link
Member

antross left a comment

By the way, have you run an end-to-end test with this in webhint?

I ask because I started digging into the callers of Configuration.loadBrowsersList and it looks like this may be a dead function. The only caller I see is Configuration.fromFilePath which itself doesn't appear to be called by anything other than tests within the repository. The CLI and VS Code extension both call Configuration.loadConfigFile directly.

I left some other comments, but we should see if someone else from @webhintio/core has any additional context on the history here. I'm thinking we may have to switch this up to do the checks in loadConfigFile instead.


if (basename === ConfigFile.PackageJson) {

const packageJson = loadJSONFile(file);

This comment has been minimized.

@antross

antross Jan 3, 2019

Member

Ah, now it makes sense :) Thanks for clarifying!

Let's add comments to the individual checks explaning which scenarios each is handling. Also, I might rename the function groupBrowsersListConfigs or similar since browserslist is the focus here.

Show resolved Hide resolved packages/hint/src/lib/config.ts
Show resolved Hide resolved packages/hint/src/lib/config.ts
@@ -1,8 +1,8 @@
# Browser configuration

`webhint` allows you to define what browsers are relevant to your
scenario by adding the property `browserslist` to your `.hintrc`
file, or in the `package.json` file. This property follows the same
scenario by adding the property `browserslist` to either your project's `.hintrc`

This comment has been minimized.

@alrra

alrra Jan 3, 2019

Contributor

Should we also look in the .browserslistrc if it exists?

This comment has been minimized.

@antross

antross Jan 3, 2019

Member

As far as populating the list goes, browerslist should do this automatically as long as we don't pass a configuration when one isn't specified (see the discussion at webhintio/rfcs#59 (comment)).

That said, this PR also adds checks to validate there aren't multiple definitions of browserslist across config files (e.g. one in .hintrc and one in package.json). Currently that only covers the files where webhint itself can also be configured.

As far as the validation checks are concerned, going beyond the current list probably requires a discussion of where else to draw the line as there's also browserslist as a valid config file as well as the BROWSERSLIST environment variable, plus the BROWSERSLIST_CONFIG environment variable which can point to an arbitrary config file somewhere else.

"version": "1.0.0",
"browserslist": ["chrome 45"]
}

This comment has been minimized.

@alrra

alrra Jan 3, 2019

Contributor
Suggested change Beta

This comment has been minimized.

@isabellachen

isabellachen Jan 4, 2019

Contributor

@antross requested for a newline at the end of the test fixtures... #1599 (comment)
I looked a some other fixtures... can't seem to discern what the standard is, some have newlines, some don't. Should I keep or remove?

This comment has been minimized.

@alrra

alrra Jan 4, 2019

Contributor

requested for a newline at the end of the test fixtures

@isabellachen Sorry for the misunderstanding. My comment was about removing the extra empty line at the end, not about the newline at the end.

can't seem to discern what the standard is, some have newlines, some don't. Should I keep or remove?

newlines should be kept.

"browserslist": ["chrome 45"]
}
}

This comment has been minimized.

@alrra

alrra Jan 3, 2019

Contributor
Suggested change Beta
@sarvaje

sarvaje approved these changes Jan 4, 2019

Copy link
Member

sarvaje left a comment

Besides the changes asking for @alrra, it looks good to me.

@antross
Copy link
Member

antross left a comment

@sarvaje @molant Do you have any context on the history of the loadBrowsersList function? It looks to be dead (per my comment copied below) meaning we're going to need to add these checks in a different place.

I started digging into the callers of Configuration.loadBrowsersList and it looks like this may be a dead function. The only caller I see is Configuration.fromFilePath which itself doesn't appear to be called by anything other than tests within the repository. The CLI and VS Code extension both call Configuration.loadConfigFile directly.

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 4, 2019

@sarvaje @molant Do you have any context on the history of the loadBrowsersList function? It looks to be dead (per my comment copied below) meaning we're going to need to add these checks in a different place.

Nope.

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 7, 2019

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.

@webhintio/core Any objections? If not I'll close this PR and webhintio/rfcs#59 tomorrow.

@molant

This comment has been minimized.

Copy link
Member

molant commented Jan 7, 2019

Maybe we should update our documentation to be clearer on that aspect and indicate the priority order and that someone could define a different configuration in .hintrc than .broserslistrc?

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 8, 2019

Maybe we should update our documentation to be clearer on that aspect and indicate the priority order and that someone could define a different configuration in .hintrc than .broserslistrc?

Agreed. Opened as PR #1611.

@antross antross referenced this pull request Jan 8, 2019

Closed

Support for .browserlistrc #59

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 8, 2019

Closing as there isn't a functional issue to fix, we're clarifying documentation in #1611, and we're okay not adding additional error messages at this time.

@antross antross closed this Jan 8, 2019

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 8, 2019

@isabellachen Sorry I didn't catch the dead code issues sooner, but thank you for working on this regardless!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment