-
Notifications
You must be signed in to change notification settings - Fork 376
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
define behavior of @import
in Cascading Style Sheet (CSS) modules
#870
Comments
I still stand by option 3 and my original comment: For me, it's less about performance advantages (theoretical or not) or ease of eng (less changes to CSS OM), but more about an advocation of less to think about/learn. Our styles are adapting to a new context/consumer, and it's just code, we can figure that part out. But, the closer it is to js modules (which the web community is certainly studying and learning intricately about), the less we'll have to teach when they reach to import CSS in their app logic. Typical UX advocate angle though I guess, I feel we should optimize for the moment the community goes to import CSS, and make that seamless. |
I have the same argument, but with the opposite conclusion. ^_^ People don't poke into the OM of a stylesheet very often anyway, and particularly don't recurse into imported sheets. But when they do, currently sheets are independent objects, and that won't change (no module graph in |
I think there's problem with current semantics and module-like semantics mixing. A constructible stylesheet is shared and can be mutated and effect every scope it's applied to. I would argue that given this setup, with a shared theme.css file: theme.js: p { color: red; } my-component.css: @import '../theme.css'; /* URLs would ideally be resolved to the referring CSS module's URL, not the document */
... my-component.js: import styles from './my-component.css'; That this code should result in the component's styles changing: my-app.js: import themeStyles from './theme.css';
themeStyles.cssRules[0].style.setProperty('color', 'blue'); because this is what would happen if the component imported |
Can you elaborate on this? Passing around a single stylesheet object lets you share, yes. But constructing a new stylesheet from the same URL will not share things; it's an independent sheet. (This is the sole unavoidable difference between css modules and constructed sheets; modules implicitly cache the top-level object, at least, by import url, so you'll get the same object from separate JS imports. But I don't think that argues for there being more differences of similar type between the two cases.) |
I am saying that when JS-importing a URL from multiple places, you get the same shared CSSStyleSheet object. I'm saying that should apply to transitive imports, otherwise you get an inconsistency depending on how you import the module. |
Right. So we have some required, but incompatible behaviors:
We can't change any of these, and if someone used to one encounters the other, they'll have problems. So the decision here is not "which behavior should we be consistent with", because we can't be consistent, it's "where do we draw the boundary between the two behaviors". I believe that there's no particularly compelling usability argument either way, as the usage of crawling and mutating stylesheet objects is fairly low in general. As such, I think the best option is to push the boundary as far in one direction as possible, so as much of the API-space is consistent as possible except for the small "quarantined" bit of inconsistency we can't remove. Given the current spread of behaviors, I think the right choice is to keep the "everything is a distinct object" behavior as much as possible, containing the "shared objects" behavior to happen only at the JS import API; sheets imported lower down aren't shared across imports. This keeps the overall semantics as consistent as possible; JS itself caches JS-imported sheets by url, but beyond that everything remains the same as it always has. |
I have some concerns in addition to the questions of distinct stylesheet identity discussed by @tabatkins and @justinfagnani above, and I am not entirely sure that we need to commit to either of these options at this point. For option 2 as stated in this post (CSS modules are leaf nodes), there are really two sub-options: 2a) Pull in the full 2b) Don't block module Evaluation on On the other hand Option 3 (CSS modules are non-leaf; If forced to choose between these options now I think I prefer (2a); it gets us close to the However, I want to take a step back and ask if we really need support for I believe there was a concern that removing So, if explicitly throwing/failing on |
I actually assumed we'd go with 2c) block module evaluation on loading the @imports, but don't fail the module if an @import fails. That way by the time you see the stylesheet, it's as fully loaded as it's going to be. Failing the module due to a failed @import is a change from normal stylesheet behavior, where the rest of the stylesheet continues to work if an @import fails. |
I would like to advocate for option (1). Reasons why:
I reviewed the many useful and interesting comments on this Stack Overflow page. My summary of the use cases:
The first use-case can I think be solved with developer build-time tooling and is about developer ergonomics only. The second use case is, I believe, about loading low-priority stylesheets later than the main ones, and can be solved by factoring these style sheets into later loads after the initial render. |
Oh BTW, CSSStyleSheetReplaceWithImport was only added a few days ago, and does not yet have data from the stable channel. |
This was discussed in a TPAC breakout session today: https://www.w3.org/2020/10/29-components-minutes.html We're not really any closer to making a decision on whether an @annevk had a concern that blocking The remaining question then is how to block The Constructable StyleSheets spec https://wicg.github.io/construct-stylesheets doesn't appear to have been updated, but @mfreed7's comment here summarizes the state of things after the most recent discussion about this in the CSS Working group (notes inlined in this comment). Whatever we do with CSS modules should align with the behavior of
|
Thanks @dandclark, that sounds like what I heard at the meeting also. I agree with all three of your points. For #3, @emilio had some good feedback into the change to Constructable Stylesheets, pushing for the current “no error” implementation. Perhaps he can comment here about this. |
I think my main argument for making it not error is that that's how most other CSS errors work, in particular parser errors and other kind of "syntax that doesn't (yet, maybe) work", like unknown at-rules and such. In particular, if someone writes syntax that right now isn't accepted by the parser and we later start accepting it, we'd start throwing, which seems bad. After a bit of testing, browsers seem to be very lenient with That alleviates a bit my concern, though it is still a problem with other things we want to do with |
+1 I agree with the suggestion that we ignore imports and emit a console warning. Clear and keeps things working otherwise. |
couldn't tell where else to ask, but this introduces a great practice to dynamically set styles. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as abuse.
This comment was marked as abuse.
Literally nobody uses |
This is important because one of the main reasons someone could want to move away from doing this, this.shadowRoot.innerHTML = `
<div> ... </div>
<style>
@import "/css/animations/wobble.css" /* (build tools not aware of arbitrary code in strings) */
div {
animation: 1s ease wobble;
}
</style>
` in their custom elements, is that moving to using JavaScript F.e. they could easily change that code to this: /* me-el.css */
@import "/css/animations/wobble.css" /* build tools inline official syntax */
div {
animation: 1s ease wobble;
} import sheet from "./my-el.css" with { type: 'css' } // build tool inlines
// ...
this.shadowRoot.innerHTML = `
<div> ... </div>
`
this.shadowRoot.adoptedStyleSheet = [sheet] At the same time, the workaround is not very difficult, they could also write the following to get things working with CSS Modules as spec'd today (f.e. in Chrome): /* me-el.css */
div {
animation: 1s ease wobble;
} import wobble from "/css/animations/wobble.css" with { type: 'css' } // build tool inlines
import sheet from "./my-el.css" with { type: 'css' } // build tool inlines
// ...
this.shadowRoot.innerHTML = `
<div> ... </div>
`
this.shadowRoot.adoptedStyleSheet = [wobble, sheet] but the workaround feels less aligned with the intrinsics of CSS. Perhaps making an If we agree to keep This might get more people further along today, than having them wait on debating how to change |
Give them 3 more years, and eventually, they will settle on an implementation. Hopefully, Webpack will be considered legacy by then. btw, in the time this issue was open, ChatGPT came out, and NVIDIA moved the NASDAQ by 2% iteself |
There was a long discussion running from #759 (comment) (in which @dandclark lays out three options) through #759 (comment) on what the behavior of
@import
in CSS modules should be. This discussion didn't lead to a resolution.Since that issue feels like it's getting too large for a single issue, I'm splitting it out into its own issue. (Sorry if that's not the preferred way of dealing with things here.)
The lack of a resolution of that issue is becoming problematic. Because Constructable Stylesheets wants to match the behavior of CSS modules, this led to a extended discussion in last week's CSS teleconference that could have been avoided if this issue had been settled. And as far a I can tell what it's waiting on is primarily the choice between whether option 2 or option 3 is preferable.
Personally I found @tabatkins's arguments in favor of option 2 relatively persuasive, because it requires less change to the CSS OM, and because many of the theorized advantages of option 3 address issues already handled in existing
@import
handling (even if it's not well specified today). (And I'd note that CSS always has the option of introducing a new syntax for doing an import as a module.)cc @matthewp @justinfagnani @emilio @argyleink @domenic @rniwa @chrishtr who also participated in that discussion.
I'd note that I'm also filing this as an effort to get w3ctag/design-reviews#405 finished up.
The text was updated successfully, but these errors were encountered: