Skip to content

Add env option to control setting the browserslist environment #1012

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

Merged
merged 6 commits into from
Jul 10, 2023

Conversation

BPScott
Copy link
Contributor

@BPScott BPScott commented Jun 24, 2023

Sometimes you have multiple browserslist environments that you'd like to compile for. This PR adds an env option to postcss-preset-env to allow you to specify which browserslist environment you would like to read from, and passes that information into browserslist's env and autoprefixer's env options.

This is like using the BROWSERSLIST_ENV environment variable, except at a function call level rather than per-process. See Babel's browserslistEnv option for a similar idea.

This allows you to have a .browserslistrc that looks like

# Exact queries aren't important.
# Just that one set of browsers is stricter than the other.
[production]
defaults

[evergreen]
last 2 versions, not dead

and then by you can specify env: 'production' or env: 'evergreen' to pick which set of browsers you would like to target

Sometimes you have multiple browserslist environments.
This commit adds an env option to allow you to specify which environment
you would like to read from, and passes that information into
browserslist and autoprefixer's `env` options.

This is like using the BROWSERSLIST_ENV environment variable, except it
can be modified at runtime. See Babel's `browserslistEnv` for a similar
idea but for Babel
Copy link
Member

@romainmenke romainmenke left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @BPScott 🎉

Can you take a look at my review comments?


This change will not rollout soon because we are in the middle of changing the license to MIT-0.

We first want to migrate to the new license and gather feedback on that change.

https://preset-env.cssdb.org/blog/license-change/

It will be more complex to do a rollback of the license change if we also have feature changes :)

@BPScott
Copy link
Contributor Author

BPScott commented Jun 24, 2023

Thanks for the feedback @romainmenke, I'll address all those. In particular thank you for the docs suggestions, I wasn't sure how to to best teach this in the docs. I wasn't confident regarding how to reframe/discourage the usage of browsers in light of this new option.

Happy to wait for a bit for the licence changes to settle, I totally appreciate the idea of avoiding awkwardness from mixing license and code changes in a single release.

When digging through the postcss-load-config docs I found that env is used as a value that is available when you define config as a function - it defaults to process.env.NODE_ENV and in postcss-loader this gets set to webpack's mode value. I initially chose env as a name because it matches the option in browserslist and autoprefixer, but I worry that having two env keys close to each other has the potential for confusion. I'm strongly considering renaming this option to browserslistEnv to avoid ambiguity. What you you think to that new name?

@BPScott
Copy link
Contributor Author

BPScott commented Jun 24, 2023

PR comments addressed.

I think the last remaining things from my side are:

  • addressing if we want to rename env to browserslistEnv to avoid ambiguity with postcss-load-config's context's env option and make it clearer that this is used in browserslist. I'm leaning towards making this change. What do you think?
  • Any other thoughts on how we teach usage of env vs browsers in the docs?
  • Is there any other corner of documentation that I've overlooked?
  • Is that the right change to the CLI? I don't understand what that code does but when searching for usages of browsers that array seemed like a big list of options so I felt like I should add my new option in there too

@romainmenke
Copy link
Member

romainmenke commented Jun 24, 2023

addressing if we want to rename env to browserslistEnv to avoid ambiguity with postcss-load-config's context's env option and make it clearer that this is used in browserslist. I'm leaning towards making this change. What do you think?

I actually think it is better that these overlap.
In practice they will mean the same thing for most people : development vs production.

It also requires a manual step to connect these two.
There is no magic (as far as I can tell) that sets the env plugin option when having a specific value in process.env.NODE_ENV. process.env.NODE_ENV happens to be exposed on a key that is named env in ctx.

I also think it is valuable to have the same option name as autoprefixer, babel, ...

There will be small number of people who do not understand how the tools work and who are doing overly complicated setups. For them it will backfire, but I also think this is a good thing. It will force them to halt and either do more research or to reach out to others who understand the tooling better.

TL;DR;

it will work intuitively for almost everyone if we use env.


Any other thoughts on how we teach usage of env vs browsers in the docs?

I think the wiki is a good place to add more in depth docs.
The README.md for postcss-preset-env is already too long and we are long past the point where we need a real documentation site :)

The wiki is good place to explain things in detail without exploding the README.md further. If you are interested in that then you could open an issue with a proposal for a wiki doc.

I personally don't think there is an immediate need for this, but I am also sure it will help some to have more docs.


Is there any other corner of documentation that I've overlooked?

No, I think you found every important bit 😄
Very thorough update 🎉


Is that the right change to the CLI? I don't understand what that code does but when searching for usages of browsers that array seemed like a big list of options so I felt like I should add my new option in there too

The code you found is configuration for two things :

  • CLI input validation
  • CLI help text

I've made a small adjustment and updated the README.md of the CLI.


I've also made another tweak to this PR.

env and browsers shouldn't be set at the same time and this should be enforce in code.
I've changed the code so that env is set to undefined when browsers is set.

I've also updated the documentation to reflect this.

Thoughts on this?
Maybe I overlooked a use case where having both is valid?

@BPScott
Copy link
Contributor Author

BPScott commented Jun 26, 2023

addressing if we want to rename env to browserslistEnv to avoid ambiguity with postcss-load-config's context's env option and make it clearer that this is used in browserslist. I'm leaning towards making this change. What do you think?

I actually think it is better that these overlap. In practice they will mean the same thing for most people : development vs production.

Excellent point that development vs production is probably the predominant use-case.

I got so wrapped up it my need of "I have 2 production builds that target broad support vs recent browsers and we do user-agent sniffing to send recent browsers a less-aggressively transpiled build" that I forgot that my use-case is niche. I like the framing that if you have a second browserslist environment then it will very likely be development.

I'm happy to leave the naming as it is :)

I've also made another tweak to this PR.

env and browsers shouldn't be set at the same time and this should be enforce in code. I've changed the code so that env is set to undefined when browsers is set.

I've also updated the documentation to reflect this.

Thoughts on this? Maybe I overlooked a use case where having both is valid?

Reading browserslist's js api docs it stats "If a query is missing, Browserslist will look for a config file". Thus I believe if both browsers and env are set then then browserslist will prefer using browsers anyway and so explicitly unsetting env when browsers is set in postcss-preset-env won't have any effect.

Because of that I left out explicitly unsetting it originally, but I'm ok with explicitly unsetting env to make that behaviour clearer.

Adding the docs to call this out that browsers takes precedence over env is great, thank you!


Thanks for the feedback and additions.
No outstanding concerns/questions from me.

Copy link
Member

@romainmenke romainmenke left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you again for this @BPScott 🎉

@romainmenke romainmenke merged commit 6aad261 into csstools:main Jul 10, 2023
@romainmenke
Copy link
Member

@BPScott This was just released as part of 9.1.0
Thank you again for your contribution

@BPScott BPScott deleted the add-env-option branch July 25, 2023 00:01
@BPScott
Copy link
Contributor Author

BPScott commented Jul 25, 2023

Fantastic, thank you @romainmenke!

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

Successfully merging this pull request may close these issues.

2 participants