Skip to content

[postcss-custom-properties] issue when used with postcss-html (document node) #595

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

Closed
2 of 3 tasks
lubomirblazekcz opened this issue Aug 30, 2022 · 11 comments
Closed
2 of 3 tasks
Labels

Comments

@lubomirblazekcz
Copy link

lubomirblazekcz commented Aug 30, 2022

Bug description

Custom properties are not transformed correctly with postcss-html, this might be problem with document node support as mentioned in ota-meshi/postcss-html#68

Source CSS

 :root {
    --blue: blue
  }
  
  body td {
      color: var(--blue)
  }
  
  @media all and (max-width: 600px) {
      body {
          color: red;
      }
  }

Expected CSS

:root {
    --blue: blue
  }
  
  body td {
      color: blue
  ;
      color: var(--blue)
  }
  
  @media all and (max-width: 600px) {
      body {
          color: red;
      }
  }

Actual CSS

 :root {
    --blue: blue
  }
  
  body td {
      color: var(--blue)
  }
  
  @media all and (max-width: 600px) {
      body {
          color: red;
      }
  }

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

No response

Debug output

No response

Extra config

No response

What plugin are you experiencing this issue on?

PostCSS Custom Properties

Plugin version

12.1.8

What OS are you experiencing this on?

macOS

Node Version

18.6.0

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
@lubomirblazekcz lubomirblazekcz changed the title [postcss-custom-properties] issue when used with postcss-html (document node not supported) [postcss-custom-properties] issue when used with postcss-html (document node) Aug 30, 2022
@romainmenke
Copy link
Member

@lubomirblazekcz Can you provide a full reproduction?

The CSS on its own works fine :

 :root {
    --blue: blue
  }
  
  body td {
      color: var(--blue)
  }
  
  @media all and (max-width: 600px) {
      body {
          color: red;
      }
  }

becomes :

 :root {
    --blue: blue
  }
  
  body td {
      color: blue
  ;
      color: var(--blue)
  }
  
  @media all and (max-width: 600px) {
      body {
          color: red;
      }
  }

@lubomirblazekcz
Copy link
Author

It's mentioned in ota-meshi/postcss-html#68, with that html you get the non-working result.

@ota-meshi mentioned that it's actually a problem with document nodes, which postcss-custom-properties might not support. I don't know if that is something that can be fixed though.

@romainmenke
Copy link
Member

I have no idea how to add support for document nodes (or what they are) :/
I will need to look into that.


A quick check does reveal that Once is called two times.
This is unexpected as the example in the other issue only processes one thing and only does this once.

It does this for the <style> tag and the style attribute on <img src="https://via.placeholder.com/160x60"...

On Once we collect all the values for custom properties.

@lubomirblazekcz
Copy link
Author

Ok no worries, I've actually came with a workaround for my usecase. But I've created this issue in case if it is something that can be fixed perhaps :)

@romainmenke
Copy link
Member

romainmenke commented Aug 30, 2022

@ai I missed Document in the PostCSS API but it does seem interesting and relatively simple to support.

We still have a few plugins with Once like custom-properties because these attempt to mimic how CSS works in a browser.

This example can't be processed with the new visitor API.
To mimic browser behaviour we first need to gather all custom properties.

.a {
	color: var(--color);
}

:root {
	--color: red;
}

I can update our plugin to use Document together with Once and combine all nodes with type root.

Once: (root) => {
	customProperties = getCustomPropertiesFromRoot(root, { preserve });
},

becomes :

Once: (root) => {
	customProperties = getCustomPropertiesFromRoot(root, { preserve });
},
Document: (document) => {
	customProperties = new Map();
	document.nodes.forEach((node) => {
		if (node.type !== 'root') {
			return;
		}

		for (const [key, value] of getCustomPropertiesFromRoot(node, { preserve }).entries()) {
			customProperties.set(key, value);
		}
	});
},

Event callback order and counts become for the original example :

once
once
document

Once is called twice, one time for each root node, but Document is called only once and it is called after Once.

This might work in practice.


Because I don't know the API I also don't know if this will cause issues with other syntaxes or plugins.

The docs also say that this API is still experimental.
Is it safe to use in something like postcss-preset-env?

Any insights you might have would be greatly appreciated 🙇

@romainmenke
Copy link
Member

romainmenke commented Aug 30, 2022

@lubomirblazekcz I just ran this through our test suite and it would require us to bump the minimum PostCSS version for our plugins.

Unknown event Document in postcss-custom-properties. Try to update PostCSS (8.2.15 now).

We keep this version relatively low because some frameworks pin the PostCSS version, making it impossible for end users to correctly manage their own dependencies. Keeping it low means there is more margin for error in those frameworks.

Update : this was added in 8.3 : https://github.com/postcss/postcss/releases/tag/8.3.0

@lubomirblazekcz
Copy link
Author

Ok, I understand. Maybe it can be updated in the future then :)

@ai
Copy link

ai commented Aug 31, 2022

Initially Document was added as hack to postcss-html, then we added it as official API since it is an useful thing

@romainmenke
Copy link
Member

thank you @ai


I think it makes sense for postcss-preset-env and the contained plugins to support the Document event. Else we risk splitting the ecosystem.

It will take a while before we have the capacity for this as this will be a very large change. We would need to update our test suite and every plugin that uses Once or OnceExit.

@romainmenke romainmenke removed this from the PostCSS Preset Env v8 milestone Jan 23, 2023
@romainmenke
Copy link
Member

There doesn't seem to be a lot of interest from the community in getting this fixed.
Closing for now, but happy to re-open and do the work here if the upstream blockers are resolved.

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

3 participants