csstools / postcss-plugins

PostCSS Tools and Plugins
https://preset-env.cssdb.org/
MIT No Attribution
890 stars 71 forks source link

PostCSS Custom Properties plugin hangs on cyclic dependencies declared in non-`:root` rules #1356

Closed reesscot closed 5 months ago

reesscot commented 5 months ago

Reproduction link

https://stackblitz.com/edit/postcss-custom-properties-bug

Bug description

When the PostCSS Custom Properties plugin is run on a CSS string with a large number of custom properties, the process completely hangs with no output.

Screenshot showing it unresponsive.

CleanShot 2024-04-10 at 14 08 27@2x

When run with two less lines of CSS custom properties, it compiles correctly: CleanShot 2024-04-10 at 14 10 24

Related: https://github.com/aws-amplify/amplify-ui/issues/5143

Actual Behavior

Node process hangs

Expected Behavior

CSS should be output, as happens when a few custom property uses are removed from the CSS string.

Can you reproduce it with npx @csstools/csstools-cli <plugin-name> minimal-example.css?

Yes

npx Output

None, the process completely hangs.

Extra config

None, using postcss-custom-properties by itself.

What plugin are you experiencing this issue on?

PostCSS Custom Properties

Plugin version

13.3.6

What OS are you experiencing this on?

Windows

Node Version

18.18.0

Validations

Would you like to open a PR for this bug?

romainmenke commented 5 months ago

Hi @reesscot,

The stackblitz link doesn't work. Do you have an alternate link?

romainmenke commented 5 months ago

Also, can you provide information as text, not as screencasts? I can't copy/paste from images or video, so takes more of my time to debug this when I don't have all the info as text :)

reesscot commented 5 months ago

@romainmenke Sorry about that, I changed the url of the stackblitz and forgot to update it here. Updated above, but here's the link: https://stackblitz.com/edit/postcss-custom-properties-bug

Let me know if you still need anything else, I didn't want to dump the enormous CSS text into this ticket.

romainmenke commented 5 months ago

As far as I can tell you got circular references between this massive amount of props.

:root,
[data-amplify-theme] {
    --amplify-components-input-color: var(--amplify-components-fieldcontrol-color);
}

/* .... */

.amplify-input {
    --amplify-components-fieldcontrol-color: var(--amplify-components-input-color);
}

We normally guard against this, and I don't know why it hangs instead of throwing errors. Will investigate, but you can remove the circular references to get unblocked.

romainmenke commented 5 months ago

Minimal repro:

:root,
[data-amplify-theme] {
    --amplify-components-input-border-color: var(--amplify-components-fieldcontrol-border-color);
}

.amplify-input {
    border-color: var(--amplify-components-fieldcontrol-border-color);
    --amplify-components-fieldcontrol-border-color: var(--amplify-components-input-border-color); /* this line is bogus CSS anyway */
}

abstract repro:

:root {
    --a: var(--b);
}

.foo {
    color: var(--b);
    --b: var(--a);
}
romainmenke commented 5 months ago

We don't guard against cyclic dependencies that are created in rules with selectors other than :root, html, ...

This is going to take some time to get sorted. But you can also just clear up your cyclic dependencies between props.


Also consider poking however you can internally to sponsor our open source work ;)

reesscot commented 5 months ago

I'm looking into why we are doing this, as it does seem odd and we may be able to get rid of these. However, according to the spec, it doesn't look like this is a cyclic dependency: https://www.w3.org/TR/css-variables-1/#cycles

It is important to note that custom properties resolve any var() functions in their values at computed-value time, which occurs before the value is inherited. In general, cyclic dependencies occur only when multiple custom properties on the same element refer to each other; custom properties defined on elements higher in the element tree can never cause a cyclic reference with properties defined on elements lower in the element tree. (my emphasis)

:root,
[data-amplify-theme] {
        --amplify-colors-neutral-60: hsl(210, 10%, 58%); 
        --amplify-colors-border-primary: var(--amplify-colors-neutral-60);
        --amplify-components-fieldcontrol-border-color: var(--amplify-colors-border-primary);
    --amplify-components-input-border-color: var(--amplify-components-fieldcontrol-border-color);
}

.amplify-input {
    border-color: var(--amplify-components-fieldcontrol-border-color);
    --amplify-components-fieldcontrol-border-color: var(--amplify-components-input-border-color); /* Agreed that this won't affect the line above, we may have been trying to modify elements down the cascade, I'm looking into it */
}
romainmenke commented 5 months ago

Interesting! That does offer an elegant way forwards.

I've prepared a fix for this : https://github.com/csstools/postcss-plugins/pull/1357


personal opinion: I really wish people would stop using this plugin 😅 It doesn't make sense to use it as anything other than a polyfill for very old browsers and this is no longer needed.

romainmenke commented 5 months ago

@reesscot This has been resolved. Thank you again for reporting this.

reesscot commented 5 months ago

@romainmenke Agreed that it doesn't really make sense to be polyfilling custom properties at this point given the level of browser support. I opened an issue in the nuxt tailwindcss repo if you want to give it a thumbs up.

Thank you so much for getting this fixed so quickly!