CSS `define:vars` should only add definitions to top-level components

mikestopcontinues commented 10 months ago

Astro Info

Astro                    v4.2.4
Node                     v20.11.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   hybrid
Adapter                  @astrojs/vercel/serverless
Integrations             auto-import
                         @astrojs/mdx
                         astro-icon
                         @astrojs/partytown
                         @astrojs/sitemap
                         astro-robots-txt

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Right now, <style define:vars={{}}> adds the variable definitions to every single element in the component. This drastically increases the footprint of components, it's difficult to read, and it will cause issues if a user intentionally wants to override a local variable within the component itself.

What's the expected result?

The CSS var definitions should only be added to the top-level components.

This issue was closed before it was discussed in withastro/astro#7328.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-sdeict-dc8tm6?file=src%2Fcomponents%2FDefineVars.astro

Participation

[X] I am willing to submit a pull request for this issue.

lilnasy commented 10 months ago

define:vars is weird. It only works on the html element the compiler notices on the file. In a page with only components and no html tags, it has no effect at all.

I want to see it deprecated altogether. Maybe we can offer a function as a replacement.

-<div>
+<div style={defineVars({ color })}>
  1111
  <button type="button">button</button>
</div>

-<style define:vars={{ color }}>
+<style>
  button {
    color: var(--color)
  }
</style>

mikestopcontinues commented 10 months ago

I'm all for it!

imaginarny commented 10 months ago

I would also like to draw attention to the behavior when you actually set define:vars along with the is:inline directive on a style tag.

<style is:inline define:vars={{ cssVar: 'example' }}>...</style>

Compiles to:

<style>--cssVar: 'example';...</style>

This way, you can't use the variable as it was inlined as-is right into the style tag. All you need would be if it either:

added :root {} wrapper around the variables
or offered some way/option that lets you define your own element/wrapper

The first option would be perfect for use in <head> with inline styles, for example, when you want to set something immediately that you also preload. You can achieve this another (better) way, though.

Although, I don't see a use case for it outside of the <head>. Just wanted to point out another "questionable" behavior while I was trying to pass a variable used in head and found this open issue among the many closed ones.

I want to see it deprecated altogether. Maybe we can offer a function as a replacement.

In the case of deprecating it, how would the function work for the <script> tag?

lilnasy commented 10 months ago

In the case of deprecating it, how would the function work for the Githubissues.
Githubissues is a development platform for aggregating issues.

withastro / roadmap