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

Regression from 6.x -> 7.x: info() not picking up browserlist configuration #838

Closed
harriha opened this issue May 11, 2017 · 17 comments
Closed
Milestone

Comments

@harriha
Copy link

harriha commented May 11, 2017

Hi,

At first this looked like a more serious issue than it turned out to be after investigation, but thought to report this anyway. Makes debugging difficult at least, spent a good while thinking my browserslist config isn't being picked up at all.

To clarify: the actual functionality seems to work fine when used as per design via postcss.

Short version: In 7.x, autoprefixer().info() with no arguments uses the default browserslist configuration instead of picking up the config from package.json or from the configuration file.

To reproduce:

In package.json (or in a separate configuration file), have something custom defined:

  "browserslist": [
    "Edge 15"
  ]

Then, run the following code and compare the output between latest 6.x and 7.x versions.

var autoprefixer = require('autoprefixer');

console.log(autoprefixer().info());

On the last 6.x release (6.7.7), the output is as predicted:

$ node index.js
Browsers:
  Edge: 15

These browsers account for 0% of all users globally

At-Rules:
  @viewport: ms

Selectors:
  ::placeholder: ms

Properties:
  user-select: ms
  hyphens: ms
  // ....and so on

Values:
  grid: ms
  inline-grid: ms

On 7.0.1 the output shows that browserslist defaults are being used:

$ node index.js
Browsers:
  Chrome for Android: 57
  Firefox for Android: 52
  And_qq: 1.2
  UC for Android: 11.4
  Android: 56, 4.4.3-4.4.4, 4.4
  Baidu: 7.12
  Bb: 10, 7
  Chrome: 58, 57, 56, 49
  Edge: 15, 14
  Firefox: 53, 52
  IE: 11, 10
  IE Mobile: 11, 10
  iOS: 10.3, 10.0-10.2
  Opera Mini: all
  Opera Mobile: 37, 12.1
  Opera: 44, 43
  Safari: 10.1, 10
  Samsung: 4

These browsers account for 90.92% of all users globally

At-Rules:
  @keyframes: webkit
  @viewport: ms, o
  @resolution: webkit

Selectors:
  ::selection: moz
  ::placeholder: webkit, ms
  :fullscreen: webkit, moz, ms
  ::backdrop: webkit
  :read-only: moz
  :read-write: moz
  :any-link: webkit, moz

Properties:
  box-shadow: webkit
  animation: webkit
  animation-name: webkit
  animation-duration: webkit
  animation-delay: webkit
  animation-direction: webkit
  // ...and so on

If calling the .info() with explicit from option (autoprefixer().info({ from: '.' })), things work in 7.x as well.

As far as I figured out, the issue seems to come from the fact that path option given to browserslist is undefined, but browserslist falls back to the default path (.) only if the path prop is missing completely.

Another thing I noticed with .info() is that, even if called for an autoprefixer plugin instance, it doesn't use the options of that instance, but rather always the given opts (ref). I feel this is a bit unintuitive as the function is meant for debugging purposes - I'd assume it to show me the exact same configuration as used by the created instance.

@ai
Copy link
Member

ai commented May 11, 2017

Ouh. This is because info() doesn’t get path to file. Thanks for report.

Do you want to send PR with from option.

@ai ai added the enhancement label May 11, 2017
@riophae
Copy link
Contributor

riophae commented May 11, 2017

I'd like to work on this :D

@ai
Copy link
Member

ai commented May 11, 2017

@riophae great! Wait for PR. Ask any question if you will have them.

@harriha
Copy link
Author

harriha commented May 11, 2017

Good to hear riophae is interested in fixing this, was about to mention that I won't likely to have time to look into this properly right now.

What comes to the fix, @ai, I was wondering why browserslist is not using the default '.' for path option more widely, meaning with null || undefined values? Defaulting to the current path more eagerly would feel like something which might often be desired. Might be that I am missing something though, or perhaps this might affect other tools using that lib somehow.

But if forgetting changing browserslist, I guess the options for the fix here would be defaulting to '.' in autoprefixer (ie. always sending a filename) or cleaning up the opts sent to browserslist to not include props with undefined as the value, allowing it to use its defaults as designed.

@riophae
Copy link
Contributor

riophae commented May 11, 2017

Hi @harriha , I was wondering the same as you. The reason why the default value is not applied when path is null or undefined, I guess is that, if the user provided a value but the value was invalid, then the user maybe won't want a default path but should be given an error. If the user is intended to use the default path then he will not use the key at all. So that's why hasOwnProperty() has been used.

@riophae
Copy link
Contributor

riophae commented May 11, 2017

I don't know if there is any better approach to have those cases well handled though.

@ai
Copy link
Member

ai commented May 11, 2017

@harriha process.cwd() as default hides a lot of mistakes. And technically it was ever part of public API and got there but accident.

So, I agree, that in this case path = process.cwd() is OK. But it other Browserslist usage, path must be explicit. At least this is tell me my experience (API design is always matter of taste).

@ai
Copy link
Member

ai commented May 11, 2017

I will look tomorrow of @riophae fix (thanks for quick PR, I was really want to release it today, but I am drunk)

@ai
Copy link
Member

ai commented May 12, 2017

Hm. I understood that from = process.cwd() by default is bad practice. Because autoprefixer(css), will not use same browserslist.

We can add from option, but it will be only in 7.1.

@ai
Copy link
Member

ai commented May 12, 2017

I removed from = process.cwd() 5671602

@ai ai closed this as completed May 12, 2017
@ai ai added this to the 7.1 milestone May 12, 2017
@ai
Copy link
Member

ai commented May 15, 2017

Released in 7.1

@zslabs
Copy link

zslabs commented May 31, 2017

Recently upgraded to 7.1.1 and am getting the default config still (in my case, adding -webkit properties everywhere). I've tried putting the config in the JS init and inside of package.json:

  "browserslist": [
    "last 2 versions",
    "ie 11"
  ],

@11bit
Copy link
Collaborator

11bit commented May 31, 2017

Hi @zslabs,
Could you provide some more details regarding your config? How do you run autoprefixer and what rules are being prefixed?

@zslabs
Copy link

zslabs commented May 31, 2017

Hi,
I'm running through gulp-postcss (latest version) and the ones I saw being impacted were box-shadow and gradient.

@11bit
Copy link
Collaborator

11bit commented May 31, 2017

@zslabs I think it is because 'last 2 versions' is very broad rule. For instance box-shadow in "Blackberry Browser 7" requires -webkit prefix and it is one of the last two.
You can try only "ie 11" and check if all the -webkit will go away.

@zslabs
Copy link

zslabs commented May 31, 2017

To clarify - these were the same settings used in 6.7.7 which did not cause this issue.

@11bit
Copy link
Collaborator

11bit commented May 31, 2017

@zslabs yep, you are right. It was changed in Browserslist 2.0 (part of Autoprefixer 7.x release). Before that 'last n browsers' was actually 'last n popular' but now it adds prefixes for all browsers including uncommon ones like Blackberry.
There is more info here: browserslist/browserslist#98

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

No branches or pull requests

5 participants