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

postcss-preset-env dynamically skips plugins with importFrom/exportTo based on browser lists #140

Closed
3 tasks done
romainmenke opened this issue Jan 9, 2022 · 23 comments
Closed
3 tasks done
Labels

Comments

@romainmenke
Copy link
Member

romainmenke commented Jan 9, 2022

Bug description

preset-env is a way to dynamically toggle plugins on and off based on config.

This conflicts with importFrom/exportTo and maybe other options where it is always required to run a certain plugin.


I don't see a way for these kinds of options to ever work correctly with postcss-preset-env.

Source CSS

.test-custom-properties {
	order: var(--order);
}

@media (--narrow-window) {
	.test-custom-media-queries {
		order: 2;
	}
}

.test-custom-selectors {
	&:--heading {
		order: 3;
	}
}

Expected CSS

:root {
  --order: 1;
}

.test-custom-properties {
	order: var(--order);
}

@media (max-width: 40rem) {
	.test-custom-media-queries {
		order: 2;
	}
}

h1.test-custom-selectors,h2.test-custom-selectors,h3.test-custom-selectors,h4.test-custom-selectors,h5.test-custom-selectors,h6.test-custom-selectors {
		order: 3;
	}

Actual CSS

.test-custom-properties {
	order: var(--order);
}

@media (max-width: 40rem) {
	.test-custom-media-queries {
		order: 2;
	}
}

h1.test-custom-selectors,h2.test-custom-selectors,h3.test-custom-selectors,h4.test-custom-selectors,h5.test-custom-selectors,h6.test-custom-selectors {
		order: 3;
	}

Does it happen with npx @csstools/csstools-cli <plugin-name> minimal-example.css?

No response

Extra config

options: {
	browsers: 'chrome >= 88',
	importFrom: {
		customMedia: {
			'--narrow-window': '(max-width: env(--sm))'
		},
		customProperties: {
			'--order': '1'
		},
		customSelectors: {
			':--heading': 'h1, h2, h3, h4, h5, h6'
		},
		environmentVariables: {
			'--sm': '40rem'
		}
	},
	stage: 0
}

What plugin are you experiencing this issue on?

PostCSS Preset Env, PostCSS Custom Properties

Plugin version

latest

What OS are you experiencing this on?

No response

Node Version

latest

Validations

  • Follow our Code of Conduct
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Would you like to open a PR for this bug?

  • I'm willing to open a PR
@equinusocio
Copy link

equinusocio commented Jan 11, 2022

In my environment importFrom is completely broken now, i'm using postcss-preset-env@7+. I use the importFrom key to inject custom environmentVariables (env()) properties inside the CSS, but from postcss-preset-env@7 they are not parsed anymore.

@Antonio-Laguna
Copy link
Member

Antonio-Laguna commented Jan 11, 2022

@equinusocio we've been discussing importFrom, mostly internally so we don't really have much to share yet. However, I'm really curious to know if you could get some minimum example that's not working as you'd expect to assess what could be going on.

Thanks a lot for taking the time to chime in here!

@romainmenke
Copy link
Member Author

romainmenke commented Jan 11, 2022

@equinusocio Preferably in a separate issue if possible as it is likely separate from the case describe here.

This issue is more about how postcss-preset-env can skip plugins if allowed by browser lists.

That will make it easier to follow up your case :)

@romainmenke romainmenke changed the title postcss-preset-env does not correctly support importFrom/exportTo in plugins that have this feature postcss-preset-env dynamically skips plugins with importFrom/exportTo based on browser lists Jan 11, 2022
@romainmenke
Copy link
Member Author

romainmenke commented Jan 11, 2022

@Antonio-Laguna I think we might need to resolve this sooner than later.
With cssdb 5.1 the support stats seem to have shifted in way that this is happening for more people.

This also just broken our CI (see last couple commits)

@equinusocio I was wrong, it's exactly the same issue :)
Setting browserslist to slightly older browsers might resolve this until we ship a fix.

@romainmenke
Copy link
Member Author

More details for env-function :

I think we need to split this one up :

  • one plugin for the side effects only (not included in preset-env)
  • one plugin that is a true polyfill : only these real values

The same approach might be needed for all these plugins :/

romainmenke added a commit that referenced this issue Jan 12, 2022
@equinusocio
Copy link

equinusocio commented Jan 12, 2022

Setting browserslist to slightly older browsers might resolve this until we ship a fix.

Here the replication on codesandbox

https://codesandbox.io/s/eloquent-northcutt-fwee7?file=/postcss.config.js:120-1199

I can confirm that setting a browserslist in package.json breaks the custom environmentVariables. If you remove it you can see a blue "hello word" box with padding. This is the browserslist we use, but I can't find versions that make it work again. Apparently, the only fix is to remove the browserslist completely.

  "browserslist": [
    "last 2 Chrome versions",
    "last 2 Firefox versions",
    "last 2 Safari versions",
    "last 2 Edge versions"
  ],

Also leaving the browserslist with the default values fix it:

    "> 0.2%",
    "not dead",
    "not op_mini all"

I saw the fix, I would like to test it if you can release it as alpha/beta/next channel.

@romainmenke
Copy link
Member Author

There is no alpha/beta/next channel.
But we hope to release this shortly.

Please be aware that the fix we have to roll out for this goes against the purpose of postcss-preset-env. We have no idea yet how we will remove this functionality in a next major version, but it will be removed.

My personal preference at this time is to extract the import / export features into different plugins which will not be included in postcss-preset-env

We do have to consider the maintenance cost of this as just keeping preset-env healthy and up to date is already a lot of work :)

Maybe there are existing alternatives to do this kind of injection?

@equinusocio
Copy link

equinusocio commented Jan 12, 2022

Yeah, i agree, just give us the time to migrate. Wanda design system is built on top of this functionality and it's also a big project and we need time to migrate it. A temp fix is enough for now.

I would probably go for postcss-jit-props and use standard var(). Or something that replaces the values in place, since they're not dynamic

@romainmenke
Copy link
Member Author

romainmenke commented Jan 12, 2022

It's a breaking change and we don't have any plans for another major version (with this kind of breaks), so you got time 😄 This fix should keep everything stable even if anything changes in browser lists.

jit-props looks like a good fit!
I hope we find a good one for each plugin that exposes this importFrom/exportTo

@LasaleFamine
Copy link

Since I'm using this feature like @equinusocio, I would like to try to build a plugin for this using the current implementation of postcss-preset-env.
For what I can understand all the work is done here https://github.com/csstools/postcss-plugins/blob/27760744c757e84d5c5b4a287b9f3ec44c1c8902/plugins/postcss-env-function/src/index.js, am I wrong? cc @romainmenke

@Antonio-Laguna
Copy link
Member

@LasaleFamine @equinusocio this is getting released in a bit

@romainmenke
Copy link
Member Author

@LasaleFamine In theory you can just take out the entire plugin directory and move it to a new repo.

relevant bits :

You would have to move a lot of things around to "un-mono-repo" the setup.
But it is definitely possible.

@LasaleFamine
Copy link

Thank you @romainmenke, I'm going to take a look and maybe trying an initial setup.

@Antonio-Laguna
Copy link
Member

Got released as 7.2.1

@equinusocio and @LasaleFamine if you could test on your end that'd be awesome.

@equinusocio
Copy link

equinusocio commented Jan 12, 2022

There is some error (this version completely broke the pipeline):

Repruduction
https://codesandbox.io/s/eloquent-northcutt-fwee7?file=/package.json

CSS

.Hello {
  color: hsl(env(--color-blue-10));
  background-color: hsl(env(--color-blue-60));
  padding: env(--space-24);
}
./pages/index.module.css
error - ./node_modules/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[2].oneOf[4].use[1]!./node_modules/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[2].oneOf[4].use[2]!./pages/index.module.css
Error: Invalid source object: [object Object]
    at Array.map (<anonymous>)
    at Array.map (<anonymous>)
ModuleBuildError: Module build failed (from ./node_modules/next/dist/build/webpack/loaders/postcss-loader/src/index.js):
Error: Invalid source object: [object Object]
    at /sandbox/node_modules/postcss-custom-properties/dist/index.cjs:1:2182
    at Array.map (<anonymous>)
    at y (/sandbox/node_modules/postcss-custom-properties/dist/index.cjs:1:1623)
    at Object.q [as plugin] (/sandbox/node_modules/postcss-custom-properties/dist/index.cjs:1:5642)
    at /sandbox/node_modules/postcss-preset-env/dist/index.cjs:1:9852
    at Array.map (<anonymous>)
    at De (/sandbox/node_modules/postcss-preset-env/dist/index.cjs:1:9691)
    at /sandbox/node_modules/next/dist/build/webpack/config/blocks/css/plugins.js:65:63
    at plugin (/sandbox/node_modules/next/dist/build/webpack/config/blocks/css/plugins.js:30:44)
    at Processor.normalize (/sandbox/node_modules/next/node_modules/postcss/lib/processor.js:40:13)

@romainmenke
Copy link
Member Author

@equinusocio This has been fixed in a release we pushed out just now.
The code checking the options was too strict after the last change and has been relaxed.

In your case you have no options for custom properties, which is completely valid.
But we only allowed valid options for this plugin or no options at all.

@equinusocio
Copy link

equinusocio commented Jan 12, 2022

@romainmenke I'm still having the issue with 7.2.2. The error is gone but still not working with this browserslist set:

  "browserslist": [
    "last 2 Chrome versions",
    "last 2 Firefox versions",
    "last 2 Safari versions",
    "last 2 Edge versions"
  ],

It only works if I remove the browserslist completely or set it to

["> 0.2%", "not dead", "not op_mini all"]

I made a better playground to test this without nextjs, with autorefresh if you live change the package json (remove the browserslist, wait some seconds). Anyway I think the issue should be reopened at this point

https://glitch.com/edit/#!/poc-preset-env-ev

@romainmenke romainmenke reopened this Jan 12, 2022
@romainmenke
Copy link
Member Author

Was closed by merging the PR

This happens because we only currently detect one shape.

importFrom: {
  environmentVariables: environmentVariables
}

We should update this to also account for arrays.

@Antonio-Laguna
Copy link
Member

@equinusocio I've checked here: https://glitch.com/edit/#!/decisive-stone-raptor?path=package.json%3A17%3A33

New version 7.2.3 seems to have finally fixed the underlying issue.

Can you confirm on your Next app?

Thanks 🙏

@equinusocio
Copy link

equinusocio commented Jan 12, 2022

@Antonio-Laguna 7.2.3 seems fixed. 🎉

Just for sharing as a possible migration step, since we are using importFrom just to replace custom env(--<KEY>) with static values, I searched for a way to do the same without using that option (also because as you said it will be removed next major). I found a plugin called postcss-replace which can be used to do the same job without any breaking changes.

The only thing to do is to configure the regex to emulate the behaviour from postcss-preset-env:

    "postcss-replace": {
      pattern: /env\(.*?--([^\s]+?)\)/gi,
      data: environmentVariables,
    },

Unfortunately this plugin replace the keys also inside comments

This way it matches all the custom env starting with -- (the custom ones) and leaves untouched the UA defined environment variables, like env(safe-area-inset-top). Said that, if in the future UA environment variables will be written with a starting --, good luck!

@romainmenke
Copy link
Member Author

So happy this is working now 🎉


Another option is to disable the plugins with side effects in preset-env and use them directly (outside of preset-env).

That should work at least up until something breaking happens in PostCSS.

as you said it will be removed next major

I think this won't happen in the next year.
Our current focus is much more on fixing long standing bugs, increasing test coverage, increasing code quality, ...

It still happens to much that tests are green and something broke for someone, exactly like this case :)

After all that we would like to handle some feature requests.

So it's very low on our list.

@equinusocio
Copy link

equinusocio commented Jan 12, 2022

So it's very low on our list.

Good to know! Another reason is that custom envs haven't a spec yet, so it's confusing to use env(--custom-env) and env(safe-area-inset-top) which are both valid, but the second one doesn't need the --. That's why I'm searching for a new way to do this, also using a custom identifier like tkn(--custom-key) or token(--custom-key)

I think we can close this now! Appreciated the quick fix!

@romainmenke
Copy link
Member Author

Happy to help!

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

No branches or pull requests

4 participants