Skip to content
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

CSS Modules #405

Open
dandclark opened this issue Aug 8, 2019 · 10 comments
Open

CSS Modules #405

dandclark opened this issue Aug 8, 2019 · 10 comments

Comments

@dandclark
Copy link

@dandclark dandclark commented Aug 8, 2019

こんにちはTAG!

I'm requesting a TAG review of:

Further details:

  • Relevant time constraints or deadlines: None, though we're ready to move forward with an implementation in Blink.
  • I have read and filled out the Self-Review Questionnare on Security and Privacy. There are no new security/privacy concerns, as this proposal follows JavaScript/JSON modules with respect to the usage of fetch.
  • I have reviewed the TAG's API Design Principles

You should also know that...

Much of the discussion regarding this feature thus far has taken place on this issue thread, also linked above: w3c/webcomponents#759

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our GitHub repo for each point of feedback
  • open a single issue in our GitHub repo for the entire review
  • [ x ] leave review feedback as a comment in this issue and @-notify @dandclark

Please preview the issue and check that the links work before submitting. In particular, if anything links to a URL which requires authentication (e.g. Google document), please make sure anyone with the link can access the document.

¹ For background, see our explanation of how to write a good explainer.

@dandclark

This comment has been minimized.

Copy link
Author

@dandclark dandclark commented Aug 13, 2019

The documents are currently identical, but https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md should be treated as the primary version going forward. I'll update the other one to point to it. Apologies for the ambiguity.

@travisleithead

This comment has been minimized.

Copy link
Contributor

@travisleithead travisleithead commented Aug 16, 2019

@dbaron

This comment has been minimized.

Copy link
Member

@dbaron dbaron commented Sep 11, 2019

I'm currently looking at this with @kenchris in a breakout at the TAG meeting.

I think we both think this looks pretty reasonable.

I'm a little concerned about bypassing handling of @import for now -- worried that people will be disturbed by this feature not working. I'd note that at least some (maybe all?) browser CSS implementations use copy-on-write sharing for stylesheets that are @imported multiple times (whose CSSOM representation start off as thin wrappers until they're accessed). So that might argue for just continuing to use that model. On the other hand, the desire to handle @imports in a way that allows the CSSOM to modify all the instances at once seems like a reasonable desire as well -- and it seems reasonable to do CSS modules such that @imports share the CSSStyleSheet objects, they're all part of the module graph, and any stylesheet that's part of the module graph just has a null parent style sheet even if it's actually imported in one or more places. It's a slightly bigger change to the model, but it does have benefits.

It's not clear to me what the benefit of deferring the decision is. Is there particular evidence you're waiting for to settle this? If not, it seems like it might be better to hammer out the decision sooner rather than crippling part of the feature just because of failure to agree on which path to take.

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Sep 26, 2019

Apple raised a security issue with JSON modules that also applies here. The importer cannot enforce that the result is actually a CSS module (or at least something that does not execute script), meaning that if the distributor went rogue or got compromised they could execute JavaScript/Wasm in your origin.

@annevk

This comment was marked as outdated.

Copy link
Member

@annevk annevk commented Sep 26, 2019

Maybe, this is probably not the place to do design work. I mainly wanted to highlight the security issue so the TAG can take that into consideration when reviewing other work.

@dbaron

This comment has been minimized.

Copy link
Member

@dbaron dbaron commented Dec 3, 2019

@plinss and I are discussing this again in a breakout in Cupertino.

We'd note that there are additional variants of options 2 and 3 that could be considered, in particular:

  • an option 2a that treats a failed @import as just something that gets dropped like in the rest of CSS, rather than causing the module to fail
  • an option 3a that makes the CSSOM for such stylesheets readonly (although neither of us really like that)

Along those lines, it might be useful if the explainer says what happens when an import fails, for readers who aren't familiar enough with ES modules. (I'm not!)

We're also concerned about relying on either the file extension (given that web behavior shouldn't depend on them) or the mime type (given the security issue mentioned above) to determine that it should be CSS, so it seems like there should be some syntax differentiation for stylesheet imports.

We're curious what the status of this is, and whether you have any thoughts on this or the previous comments?

@plinss

This comment has been minimized.

Copy link
Member

@plinss plinss commented Dec 3, 2019

I presume this also works with dynamic imports? In that case is the module returned simply the CSSStyleSheet?

@kenchris

This comment has been minimized.

Copy link

@kenchris kenchris commented Dec 4, 2019

@dandclark

This comment has been minimized.

Copy link
Author

@dandclark dandclark commented Dec 4, 2019

Re: @dbaron

I agree that 2a is worth considering. I am a little wary of not failing the module graph if any resource is not found, as that somewhat goes against the expectation that if a module graph is executing, all statically imported resources are present. On the other hand the basic principle of option 2 is that we're not really changing the semantics of @import so perhaps just benignly dropping the missing @import to be consistent with the current CSS behavior makes sense. I'll update the explainer to list this possibility as well.

I'm not sure that making the stylesheet OM read-only solves a lot of the issues with option 3; we still have the problem where a stylesheet can have multiple parents if @imported from multiple sheets, and we would still need to decide the right way to reflect that in a read-only OM. Furthermore, the main motivating scenario for option 3 is that dynamic changes to a particular stylesheet can be reflected in multiple importers. If the OM is read-only this capability would be limited.

I'll add a comment in the explainer about the behavior when import fails (the module graph doesn't execute; same as with the failed import of a JavaScript module).

Your concerns about relying on file extension or MIME type are widely shared at this point. Issue thread here. We're proposing @littledan and @xtuc's https://github.com/littledan/proposal-module-attributes/ for State 1 to TC39 this week. The ideal outcome is that this will enable us to use the import syntax to specify the module type, e.g.

import styleSheet from "./foo.css" with type: "css";

Re @plinss:

I presume this also works with dynamic imports? In that case is the module returned simply the CSSStyleSheet?

Yes, it will work with dynamic imports. The returned Promise resolves with the module namespace object, where the default property is the CSSStyleSheet. More discussion on this here. That thread is about JSON modules but the ideas are the same. Justin's comment here notes that this keeps open the future possibility of things like exporting named CSS selector references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.