kimroen / ember-cli-autoprefixer

Automatically run your styles through autoprefixer
MIT License
103 stars 25 forks source link

targets.js #47

Closed amk221 closed 4 years ago

amk221 commented 4 years ago

My understanding was that ember-cli-autoprefixer took the list of browsers from targets.js. As of 1 beta, the suggestion is to use .browserlistrc.

I quite liked not having to maintain the list of browsers in multiple places.

What is the plan going forward? Is the intention that ember-cli will read from .browserlistrc too?

amk221 commented 4 years ago

Also, what do we do about a conditional list

// taken from targets.js
if (isCI || isProduction) {
  browsers.push('last 2 Edge versions');
  browsers.push('ie 11');
}
amk221 commented 4 years ago

Anybody got any input on this?

bradleypriest commented 4 years ago

Hi @amk221, I also brought this up in https://github.com/kimroen/ember-cli-autoprefixer/pull/45#issuecomment-606975158

Sounds like Martin is definitely open to README improvements, and possibly to re-adding support

amk221 commented 4 years ago

...so this means I end up with vendor prefixes for 'last 2 Edge versions' and 'ie 11' in my development builds. Prior to version 1 of ember-cli-autoprefixer, this wasn't the case. The point of the conditional in targets.js is for quicker build times in development. Can we get some insight into why this change was made?

snewcomer commented 4 years ago

@amk221 Can the logic you need be moved to ember-cli-build.js with overrideBrowserlist?

https://github.com/kimroen/ember-cli-autoprefixer/pull/45/files#diff-04c6e90faac2675aa89e2176d2eec7d8R21

amk221 commented 4 years ago

Not sure how that would work? A .browserslistrc.js would do the trick, but that’s not a thing.

Edit: you changed the comment - oh I see - it could. But it doesn’t really round the issue off very well.

snewcomer commented 4 years ago

See overrideBrowserslist.

https://github.com/kimroen/ember-cli-autoprefixer/pull/45/files#diff-04c6e90faac2675aa89e2176d2eec7d8R21

snewcomer commented 4 years ago

Actually I see the issue. This wrapper library should feed the browser list in targets.js. One sec. Let me push a PR up.

snewcomer commented 4 years ago

@amk221 How does #51 look to you? @mfeckie Do you have any thoughts?

amk221 commented 4 years ago

Seems good. I'm questioning why this was removed in the first place :)

mfeckie commented 4 years ago

@amk221 @snewcomer TL:DR -> I'm not against re-adding support for targets.js, but there are some things worth considering.

It was removed to bring the usage in line with how the underlying autoprefixer library expected to receive config. There's a conversation worth having about how these things should be handled in an Ember specific context. For example, the documentation for configuring autoprefixer expects the configuration to be read from different files This presents some possibility of confusion for someone reading the documentation for autoprefixer.

When I did the upgrade I referenced the advice in the autoprefixer docs

overrideBrowserslist (array): list of queries for target browsers. Try to not use it. The best practice is to use .browserslistrc config or browserslist key in package.json to share target browsers with Babel, ESLint and 

I can see the value of re-using the targets config from a consistency perspective, however, I don't think that this supports the full range of configuration options (https://github.com/postcss/autoprefixer#using-environment-variables-to-support-css-grid-prefixes-in-create-react-app for example, shows using different values for development vs production)

There's an argument to be made that this should be a major version bump. Anyone using the 1.x version would have their current configuration living in .browserslistrc or browserslist key in package.json, as I believe this change would mean they would be ignored.

snewcomer commented 4 years ago

I considered the Node 10 a major. The fact that config/targets.js is not passed to autoprefixer can be considered a bug.

Is your desire to pass in two different browser lists to Babel and autoprefixer? I haven't come across an app where these two configurations were different (yet :) ).

mfeckie commented 4 years ago

I'm not concerned about moving back to targets.js, I'm concerned that anyone using the current 1.x version would have any config they put in .browserslistrc or in package.json ignored if that PR gets merged. That seems like a 'breaking' level change.

snewcomer commented 4 years ago

Agreed. I think we can muster up something...order of importance -

  1. has browserlistrc file - no overrideBrowserslist
  2. has package.json browserslist config - no overrideBrowserslist
  3. assign config/targets to overrideBrowserslist.

What do you think?

mfeckie commented 4 years ago

Sounds reasonable

amk221 commented 4 years ago

Ok, thanks for the info.

I suppose the inverse might be nice... I mean, that ember-cli didn't use targets.js, but just consumed .browserlistrc.

But that aside, I'm happy to go with whatever. Keeping it how it is now, or going with your new solution.

snewcomer commented 4 years ago

@amk221 @mfeckie Mind reviewing this PR? Thank you!!

https://github.com/kimroen/ember-cli-autoprefixer/pull/51/files#diff-168726dbe96b3ce427e7fedce31bb0bcR29

snewcomer commented 4 years ago

1.0.1 released. Lmk if you have any issues!