Skip to content

Convert CSS Modules to scoped styles#38

Merged
drwpow merged 8 commits into
mainfrom
scoped-styles
Mar 30, 2021
Merged

Convert CSS Modules to scoped styles#38
drwpow merged 8 commits into
mainfrom
scoped-styles

Conversation

@drwpow
Copy link
Copy Markdown
Member

@drwpow drwpow commented Mar 30, 2021

Changes

⚠️ Edit: in the Loom, I mention the reason for making a new PostCSS plugin was 2-fold. The first is because Svelte’s implementation, while great, is not isolated very much at all from the rest of Svelte. The other (unmentioned) reason is that there wasn’t another existing PostCSS plugin I found that matches Svelte’s behavior (the old CSS Modules plugin we were using is missing a few key features).

Also as mentioned in the video, we kinda have to use PostCSS anyway because that’s still where a lot of important style libraries are maintained, like autoprefixer. And since we are using PostCSS, it does a ton of lifting that Svelte has to implement manually. Given the choice between tying yourself to Svelte’s parser, vs PostCSS, the latter seems safer.

Reasoning

  • We now get raw-element selectors, like Svelte (e.g. you can now style divs within an .astro component)
  • More predictable behavior (if you wrote class="nav" on an element, that’s now preserved with class="nav astro-gu2vWp" rather than obfuscated to class="nav-gu2vWp" beyond your control)
  • Encapsulated styling is a bit more reliable than it was before
  • Performance: ??? Unknown, but it‘s certainly not slower than before. Might be faster, but not profiled yet.

Notes

This version matches Svelte‘s implementation with one major difference: Svelte only adds scoped HTML classes where necessary, and Astro adds scoped HTML classes to all elements within a component.

For example, say you had an <h1> tag in a Svelte component. Svelte would leave it as <h1> until you wrote a style rule targeting it, then it would add the <h1 class="svelte-hxgzt2p"> class. Astro, regardless, adds a <h1 class="astro-hxgzt2p"> scoped class whether you wrote styles for it or not.

The advantage in Astro’s favor is that we can process this much more efficiently than Svelte, as we can parallelize adding HTML classes as the styles are being built (Svelte, conversely, has to build styles first, then query what‘s used over-and-over again, thereby slowing down the whole compilation). Astro‘s method is also less error-prone, as Svelte has to rely on complicated lookups within your component to tell if you‘re really using it or not (and they’ve had to fix many bugs for complex selectors like .menu:focus > ul > li + li as they‘re recreating DOM selectors 🙈). But the advantage in Svelte‘s favor (assuming it’s correct) is that the HTML payload may be smaller with a few less classes. Still, gzip cuts down on this by quite a lot (it‘s the same class over and over again per-component), so gut tells me that our approach will be faster and more reliable for more users. I’d only want to look on removing unused CSS classes as part of a larger effort, and only if it has a big payoff.

Testing

  • Tests are passing
  • Tests updated where necessary

Docs

  • Docs / READMEs updated
  • Code comments added where helpful

}

/** PostCSS Scope plugin */
export default function astroScopedStyles(options: AstroScopedOptions): Plugin {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This could maybe be its own PostCSS plugin eventually? But local for now.

'.class *': `.class${className} ${className}`,
'.class>*': `.class${className}>${className}`,
'.class :global(*)': `.class${className} *`,
'.class:not(.is-active)': `.class${className}:not(.is-active)`, // Note: the :not() selector can NOT contain multiple classes, so this is correct; if this causes issues for some people then it‘s worth a discussion
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The PostCSS plugin code may be a little hard to follow, so treat these tests as the proof. Please suggest more tests if you see any complicated stuff missing!

Copy link
Copy Markdown
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM! Loved the loom.

Your PR comment around performance is good to keep in mind, but that sounds like a nightmare and your note on the Svelte teams difficulty means I'd definitely prefer the plan outlined here, at least for now.

Comment thread src/compiler/optimize/postcss-astro-scoped/index.ts
Comment thread src/compiler/optimize/postcss-astro-scoped/index.ts Outdated
elementNodes.push(node);
// 3. add scoped HTML classes
if (NEVER_SCOPED_TAGS.has(node.name)) return; // only continue if this is NOT a <script> tag, etc.
// Note: currently we _do_ scope web components/custom elements. This seems correct?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that's right? Could be worth confirming as we start onboarding people

attr.value[k].data += ' ' + scopedClass;
} else if (attr.value[k].type === 'MustacheTag' && attr.value[k]) {
// MustacheTag
attr.value[k].content = `(${attr.value[k].content}) + ' ${scopedClass}'`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is amazing, so clean!

@drwpow drwpow merged commit ee6ef81 into main Mar 30, 2021
@drwpow drwpow deleted the scoped-styles branch March 30, 2021 16:11
jsparkdev added a commit that referenced this pull request May 12, 2026
* fix: use named HTML entities for attribute escaping

Update the attribute escaping logic to use named entities (&amp; and &quot;)
instead of numeric ones (&#38; and &#34;). This improves the readability of
the rendered HTML and follows common conventions for character references.

* changeset

* update changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants