Skip to content

Add bundler compatibility to styles plugin#92

Open
DmitrySharabin wants to merge 9 commits intomainfrom
bundler-compatibility
Open

Add bundler compatibility to styles plugin#92
DmitrySharabin wants to merge 9 commits intomainfrom
bundler-compatibility

Conversation

@DmitrySharabin
Copy link
Copy Markdown
Member

@DmitrySharabin DmitrySharabin commented Mar 20, 2026

Summary

getStyle() normalizes any style input into an options object with a ready-to-adopt css property (CSSStyleSheet or Promise<CSSStyleSheet>). This enables bundler-compatible patterns like new URL("./foo.css", import.meta.url) and CSS Import Attributes.

Changes

  • get-style.js: Single getStyle(value, baseUrl, defaults) function handling all input types:
    • CSSStyleSheet — returned as-is
    • string / URL — resolved via cssToSheet(cachedFetch(fullUrl))
    • Options dict — URL coercion, css ??= (explicit css wins over fetch), CSS string → CSSStyleSheet
    • Promise — resolves, unwraps ESM .default if present, recurses through getStyle
    • Spread order { ...defaults, ... } ensures explicit values win
  • base.js: defineStyles simplified to one getStyle() call per entry; deduplication uses options.fullUrl directly
  • util.js: Barrel exports getStyle
  • Tests: 10 declarative hTest cases covering all branches. run() is a one-liner pass-through; Promise tests verify css is a Promise resolving to CSSStyleSheet

Partially addresses #89.

🤖 Generated with Claude Code

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 20, 2026

Deploy Preview for nude-element ready!

Name Link
🔨 Latest commit a461f0c
🔍 Latest deploy log https://app.netlify.com/projects/nude-element/deploys/69c1b7bfaae4af000898b392
😎 Deploy Preview https://deploy-preview-92--nude-element.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.


for (let options of def) {
if (typeof options === "string") {
if (options instanceof URL) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not all URL instances are due to bundlers, these may come up naturally for a number of other reasons.
This seems to conflate two things:

  • Styles could be either a URL reference or an options dictionary
  • Style URL could be either a string or a URL object

There are 4 combinations of conditions here, and this only handles 3 of them. A natural way to handle this without any increase in complexity is:

  • Check if options is a URL → if so, rewrite to { url: options }
  • Check if options.url is a URL object → if so, do url = url.href (if that's even needed, most APIs that handle string URLs also handle URL instances since they naturally get coerced to url.href).

Copy link
Copy Markdown
Member Author

@DmitrySharabin DmitrySharabin Mar 23, 2026

Choose a reason for hiding this comment

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

Fixed. getStyle() handles all 4 combinations:

  • Bare URL → coerce to string, fall through to string handling
  • Bare string → { url: value }
  • Options dict with URLoptions.url = options.url.href
  • Options dict with string url → used as-is

defineStyles is now a single getStyle() call per entry.

else if (options instanceof Promise) {
// Dynamic import: import("./foo.css", { with: { type: "css" } })
// Resolves to { default: CSSStyleSheet } — unwrap .default, fall back to raw value
options = { css: options.then(m => m.default ?? m), ...defaultOptions };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is quite an assumption. Not all Promises would resolve to a CSSStylesheet. A promise could resolve to any other value we support. We should first check if it's a promise and then proceed with the resolved value as normal.

Copy link
Copy Markdown
Member Author

@DmitrySharabin DmitrySharabin Mar 23, 2026

Choose a reason for hiding this comment

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

Fixed. The Promise branch resolves first, then recurses through getStyle() — the resolved value goes through the same normalization as any other input. No assumption about what the Promise contains.

ESM default unwrapping ({ default: ... }) is scoped to the Promise .then() only, since namespace objects only appear from import() resolution — never from direct caller input.

Copy link
Copy Markdown
Contributor

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

I left some comments, but also this is getting to the point where it needs a utility function (e.g. getStyle()), rather than doing everything in base.js.

DmitrySharabin added a commit that referenced this pull request Mar 20, 2026
- Add src/plugins/styles/util/get-css.js with getCSS(value, baseUrl)
- Handles ES module default unwrap, CSSStyleSheet, string, URL
- URL branch has TODO for future CSS import attribute detection (see #89)
- Export getCSS from util.js; use it in base.js Promise branch

Addresses #92 review feedback
DmitrySharabin added a commit that referenced this pull request Mar 23, 2026
- Add src/plugins/styles/util/get-css.js with getCSS(value, baseUrl)
- Handles ES module default unwrap, CSSStyleSheet, string, URL
- URL branch has TODO for future CSS import attribute detection (see #89)
- Export getCSS from util.js; use it in base.js Promise branch

Addresses #92 review feedback
@DmitrySharabin DmitrySharabin force-pushed the bundler-compatibility branch from 1a8e61f to 2a55bc1 Compare March 23, 2026 10:36
- Handle URL, Promise, and CSSStyleSheet in styles definition (base.js)

Partially addresses #89
- Add src/plugins/styles/util/get-css.js with getCSS(value, baseUrl)
- Handles ES module default unwrap, CSSStyleSheet, string, URL
- URL branch has TODO for future CSS import attribute detection (see #89)
- Export getCSS from util.js; use it in base.js Promise branch

Addresses #92 review feedback
@DmitrySharabin DmitrySharabin force-pushed the bundler-compatibility branch from 43f9ff1 to b8ba3a3 Compare March 23, 2026 13:02
@DmitrySharabin DmitrySharabin force-pushed the bundler-compatibility branch from b8ba3a3 to dc55a24 Compare March 23, 2026 13:06
value = value.default;
}

if (value instanceof Promise) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using .then() is a little awkward. I wonder if there may be value in exporting a getStyleSync() that this calls, then this can be async (which effectively it already is)

Copy link
Copy Markdown
Member Author

@DmitrySharabin DmitrySharabin Mar 23, 2026

Choose a reason for hiding this comment

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

We tried splitting into getStyleSync() + async getStyle() — the naming was confusing (it's not "the sync version", it's a different function). Merged back to one function.

getStyle can't be async — the caller reads options.fullUrl, options.url synchronously for deduplication in defineStyles. Making it async would propagate to the hook system.

The .then() remains but the callback is now minimal: unwrap ESM .default if present, then recurse via getStyle(resolved, baseUrl).css. Recursion eliminates all duplication between sync and async paths.

}

if (value instanceof CSSStyleSheet) {
return { css: value, ...defaults };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not quite normalized yet, css could now be either a CSSStyleSheet object or a string, so we need to handle it further at call sites.

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.

Fixed. css is now always CSSStyleSheet or Promise<CSSStyleSheet>:

  • CSS strings → converted via cssToSheet() inside getStyle
  • URL-based inputs → cssToSheet(cachedFetch(fullUrl))
  • Explicit css in options dict is preserved (css ??= — only fetches when absent)

No further handling needed at call sites.

- Extract cssToSheet() from adopt-style.js into its own module
- Split getStyle() into getStyleSync() (sync paths) + getStyle() (adds Promise handling)
- Promise branch now pipes through cssToSheet() so css always resolves to CSSStyleSheet
- Export getStyleSync and cssToSheet from util.js
- Update tests: split into getStyleSync/getStyle groups, add defaults coverage

Addresses review feedback on #92
- Merge getStyleSync into getStyle (single function, no naming confusion)
- Always populate css: CSSStyleSheet or Promise<CSSStyleSheet> for URL inputs
- Compute fullUrl inside getStyle, simplify base.js deduplication
- Fix spread order: { ...defaults, css } so explicit values win
- URL inputs coerce to string and fall through (no duplicated handling)
- Promise branch uses recursion instead of manual fetch/convert pipeline
- Remove getStyleSync from public barrel export

Addresses review feedback on #92
The { default: ... } namespace only appears from dynamic import()
resolution, never from direct caller input. Moving the check inside
the Promise .then() callback eliminates false positives on options
dicts that happen to have a "default" property.
- Shared run() is a simple pass-through to getStyle()
- Non-promise group: tests for CSSStyleSheet, string, URL, options dict variants
- Promise group: explicit Promise.resolve() args, inherited check verifies
  css resolves to CSSStyleSheet
- ESM default test uses real dynamic import() via data URI
- subset: true check — only assert properties we care about
- Mock CSSStyleSheet and fetch to avoid Node/network dependencies
@DmitrySharabin DmitrySharabin requested a review from LeaVerou March 23, 2026 22:14
DmitrySharabin added a commit to color-js/elements that referenced this pull request Mar 23, 2026
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