Introduce CSS Module Script by dandclark · Pull Request #4898 · whatwg/html (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation59 Commits66 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Choose a reason for hiding this comment

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

Great start! Some refactoring to do to avoid calling JavaScript-facing constructors/functions, but overall nice and simple.

source Outdated Show resolved Hide resolved

What's the relationship between this and #4423?

What's the relationship between this and #4423?

Oh, I hadn't seen #4423. The two PRs are doing almost the same thing in a pretty similar way so if you and @littledan prefer I'm OK with dropping mine and deferring to that one since it came first. :) I'm also happy to keep driving this one if that's easier.

The one difference I see is that #4423 uses CSSStyleSheet.replace() whereas mine uses replaceSync(). replaceSync() is used here because it throws on @imports, which we want to block until we've reached an agreement on which way to handle them.

Although, given the need to work out the security issue raised by Apple, it may still be some time before either can be merged.

If you're willing to take over, that's great, and I'll close the other one. I was just wondering if I missed something.

If you're willing to take over, that's great, and I'll close the other one. I was just wondering if I missed something.

Sure, I'll keep moving forward with this one.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request

Dec 14, 2019

This is the final change required for CSS Modules to be utilized by developers. Following the acceptance of this change, if you run chromium with the CSSModules runtime flag, the following is now valid syntax:

CSS Modules Explainer: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md

CSS Modules Spec PR: whatwg/html#4898

Bug: 967018 Change-Id: Ifdee5b92259fb7e4e9c8f9aa88e69a98eb55c551 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1799923 Commit-Queue: Sam Sebree sasebree@microsoft.com Reviewed-by: Hiroshige Hayashizaki hiroshige@chromium.org Reviewed-by: Kouhei Ueno kouhei@chromium.org Reviewed-by: Hiroki Nakagawa nhiroki@chromium.org Reviewed-by: Yuki Shiino yukishiino@chromium.org Cr-Commit-Position: refs/heads/master@{#724896}

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request

Dec 14, 2019

CSS Modules Explainer: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md

CSS Modules Spec PR: whatwg/html#4898

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request

Dec 19, 2019

…s, a=testonly

Automatic update from web-platform-tests [SyntheticModules] Implements CSS Modules

CSS Modules Explainer: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md

CSS Modules Spec PR: whatwg/html#4898

wpt-commits: 6fbd872e9ac5fe60e32946bc9b318be6eeada123 wpt-pr: 20012

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request

Dec 20, 2019

…s, a=testonly

Automatic update from web-platform-tests [SyntheticModules] Implements CSS Modules

CSS Modules Explainer: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md

CSS Modules Spec PR: whatwg/html#4898

Bug: 967018 Change-Id: Ifdee5b92259fb7e4e9c8f9aa88e69a98eb55c551 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1799923 Commit-Queue: Sam Sebree <sasebreemicrosoft.com> Reviewed-by: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Reviewed-by: Hiroki Nakagawa <nhirokichromium.org> Reviewed-by: Yuki Shiino <yukishiinochromium.org> Cr-Commit-Position: refs/heads/master{#724896}

wpt-commits: 6fbd872e9ac5fe60e32946bc9b318be6eeada123 wpt-pr: 20012

UltraBlame original commit: 8c5b203f0fad559c872cc96687daa54cda3803bb

source Show resolved Hide resolved

…d be used to directly load a CSS module

Choose a reason for hiding this comment

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

Oh, dang, I think I found something. This attempts to create CSSStyleSheet objects unconditionally, right? Even in workers? But those objects aren't exposed in workers.

What is the intended behavior in workers? I think that needs a spec (and maybe tests).

Yes, seems like that's a bug in the PR.

My thinking is that the behavior in a Worker should be basically the same as it is now without the CSS module scripts feature. The import's MIME type check should fail when it sees text/css, even if type: "css" was asserted. We do have a test for this, validating that a Worker attempting to load a CSS module fails to do so: https://github.com/web-platform-tests/wpt/blob/master/html/semantics/scripting-1/the-script-element/css-module/css-module-worker-test.html

The simplest spec fix might be to just use the destination parameter in "fetch a single module script" to prevent us from trying to create a CSS module in a Worker. This step (line 90788):

If the MIME type essence of MIME type is text/css and module type is "css", then set module script to the result of creating a CSS module script given source text and module map settings object.

Could become this:

If the MIME type essence of MIME type is text/css, module type is "css", and destination is "script", then set module script to the result of creating a CSS module script given source text and module map settings object.

My thinking is that the behavior in a Worker should be basically the same as it is now without the CSS module scripts feature.

Makes sense to me! And I'm glad to hear we have a test for it.

The simplest spec fix might be to just use the destination parameter in "fetch a single module script" to prevent us from trying to create a CSS module in a Worker. This step (line 90788):

Interesting! I feel a bit unsure about using destination in such a way, but I see that the algorithm already switches on it for some fetching stuff, so maybe it's fine. An alternate approach might be to check if CSSStyleSheet is exposed in module map settings object's Realm, either in that step, or as an early-return-null in "create a CSS module script".

WDYT?

I think I prefer your version. I've added it as an early return in "create a CSS module script".

@domenic Friendly ping -- when you get a chance please let me know if the fix for the Worker issue looks good to you. It would be great to ship this soon but I'd like to have this PR in a good state first.

Yay, all merged! @dandclark can you be sure to file WebKit and Gecko bugs and update the OP with links to them appropriately?

Also let us know if you have any interest in writing a post for https://blog.whatwg.org/ about this work; it's been five years since https://blog.whatwg.org/js-modules and people might enjoy an update diving into how this work was spread across multiple standards bodies and proposals, and what's coming up next with JSON and maybe HTML modules!

Thanks @domenic!

I actually wasn't expecting that we'd complete the merge quite yet since @annevk had a concern here about adding another import assertions host hook that I don't think we'd formally resolved yet. That said I think it's fine because:

The addition of the host hook has been discussed twice now in TC39, most recently last night (I still need to update the issue thread). At this point I think it's very unlikely that the host hook would be added due to concerns from non-web EcmaScript hosts.
Even if the host hook were added and we need to update the HTML integration, I don't think it would be a breaking change for implementors. The web is always going to fail on unknown module types anyway. The host hook was more about whether we should make non-web hosts do so as well.

Yay, all merged! @dandclark can you be sure to file WebKit and Gecko bugs and update the OP with links to them appropriately?

Done.

Also let us know if you have any interest in writing a post for https://blog.whatwg.org/ about this work; it's been five years since https://blog.whatwg.org/js-modules and people might enjoy an update diving into how this work was spread across multiple standards bodies and proposals, and what's coming up next with JSON and maybe HTML modules!

Yes, I'd be interested in that.

I actually wasn't expecting that we'd complete the merge quite yet since @annevk had a concern here about adding another import assertions host hook that I don't think we'd formally resolved yet. That said I think it's fine because:

Ah, great, thanks for spelling that out. Indeed it was my impression these things were pretty settled as well.

Yes, I'd be interested in that.

Awesome! I'll reach out to you over email with some details on getting set up with an account etc.

uasan mentioned this pull request

Oct 20, 2022

Ms2ger added a commit to Ms2ger/html that referenced this pull request

Oct 20, 2023

Ms2ger added a commit that referenced this pull request

Oct 20, 2023

Ms2ger added a commit that referenced this pull request

Oct 20, 2023

Ms2ger added a commit that referenced this pull request

Oct 23, 2023

annevk pushed a commit that referenced this pull request

Oct 30, 2023

In particular:

Give script's settings object an ID and data-x.
Note that a script's fetch options can be null.
Correct miscount in definition of 'script'. This was introduced in 3d45584 (#4898).
Fix typos in HostLoadImportedModule.

Fixes #9867 and closes #9888.

lozy219 pushed a commit to lozy219/html that referenced this pull request

Mar 10, 2025

In particular:

Give script's settings object an ID and data-x.
Note that a script's fetch options can be null.
Correct miscount in definition of 'script'. This was introduced in 3d45584 (whatwg#4898).
Fix typos in HostLoadImportedModule.

Fixes whatwg#9867 and closes whatwg#9888.

bhearsum pushed a commit to mozilla-releng/staging-firefox that referenced this pull request

May 1, 2025

…s, a=testonly

Automatic update from web-platform-tests [SyntheticModules] Implements CSS Modules

CSS Modules Explainer: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md

CSS Modules Spec PR: whatwg/html#4898

wpt-commits: 6fbd872e9ac5fe60e32946bc9b318be6eeada123 wpt-pr: 20012