WICG / webcomponents

Web Components specifications
Other
4.38k stars 375 forks source link

Cascading Style Sheet module scripts #759

Closed justinfagnani closed 2 years ago

justinfagnani commented 6 years ago

In addition to HTML Modules, the ability to load CSS into a component definition is an important capability that we're currently lacking. Loading CSS is probably a more important use case judging by the popularity of CSS loaders in JavaScript bundlers.

Currently, styles are usually either defined inline with HTML templating or JSX or loaded with various JS Bundler CSS Loaders. The CSS loaders often have global side-effects like appending a <style> tag to the document, which does not generally work well with the style scoping of Shadow DOM.

I propose that we add Cascading Style Sheet module scripts to the platform, allowing CSS to be imported directly by JavaScript:

import styles from './styles.css';

Exports

The semantics for Cascading Style Sheet module scripts can be very simple, and combined with Constructable Stylesheets allow the importer to determine how the CSS should be applied to the document.

To start with, the only export of a Cascading Style Sheet module script would be a default export of the CSSStyleSheet object. This can then simply be added to document.styles or shadowRoot.styles:

import styles from './styles.css';

class MyElement extends HTMLElement {
  constructor() {
    this.attachShadow({mode: open});
    this.shadowRoot.adoptedStyleSheets = [styles];
  }
}

Additional Features

Other userland CSS module systems often have more features, like the ability to import or export symbols that are defined in the module. ie:

import (LitElement, html} from 'lit-element';
import styles from './styles.css';

class MyElement extends LitElement {
  constructor() {
    this.attachShadow({mode: open});
    this.shadowRoot.adoptedStyleSheets = [styles];
  }

  render() {
    return html`<div class=${styles.classes.exampleClass}></div>`;
  }
}

These features may be very useful, but they can be considered for addition to the CSSOM itself so that they're exposed on CSSStyleSheet and available to both Cascading Style Sheet module scripts and styles loaded via <style> and <link> tags, or constructed from a string.

Polyfilling

It's not easy to polyfill a new module type, but build-time tools can create JavaScript modules that adhere to the Cascading Style Sheet module script interface.

.exampleClass {
  color: red;
}

Can be compiled to:

// create a container and scope to hold a style sheet:
const container = document.createElement('div');
const shadowRoot = container.attachShadow({mode: 'open'});

// create a <style> element to add css text to
let styleElement = document.createElement('style');

// add the styles
styleElement.textContent = String.raw`
.exampleClass {
  color: red;
}
`;

// add the <style> to the document so it creates a StyleSheet
shadowRoot.appendChild(styleElement);

const stylesheet = styleElement.sheet;
export default stylesheet;

edit: updated to the actually Constructible StyleSheet API. edit: updated to refer to the feature by "Cascading Style Sheet module script"

matthewp commented 5 years ago

@dandclark

Option 2 vs 3 would not behave differently in terms of the the styles that are applied.

Ok, this was my misunderstanding then. I was expecting Option 3 to prevent a.css from being applied the second time. So if they are equivalent in that regard, I have no opinion on which is better.

I would love to see some future CSS feature that allows CSS modules to be used more similar to how JS modules are used, where all dependencies can be explicitly declared and applied only once.

bzbarsky commented 5 years ago

@matthewp I think I finally put my finger on the conceptual issue with that (very reasonable!) desire. For JS modules, it doesn't matter "where" in the module graph they are in terms of the multiple graph edges pointing to them. In fact, the very concept doesn't make sense. But per the above description of the desired CSS behavior, we have a graph that has two incoming edges to the same node, and we want to base the application of that node based on one, but only one, of those edges. Again, unless I am missing something...

matthewp commented 5 years ago

@bzbarsky That does sound right. I have brought up this idea before (here) (note that its a dated idea and probably would be much different with modules). Maybe we should take this discussion there, or another issue if you prefer. I don't want my misunderstanding of the options to derail this thread.

justinfagnani commented 5 years ago

To add customer requests for the use-case I'm referring to:

We have a current Polymer customer who is using a feature we call "style includes" which are basically a direct replacement for @import (a <style> tag can include another style with an include attribute. We do this so we can process the styles for polyfilling) ask to be able to dynamically replace a certain include across their app in order to implement theme switching.

It would be very natural to do this with CSS modules with Option 3 and @import. With Option 2 they would have to import all switchable/base styles directly via the JS module system, rather than use @import in CSS. This would hurt the ability to refactor the base styles because each JS module has to import all transitive CSS dependencies as direct dependencies, rather than getting them transitively via their immediate CSS imports.

emilio commented 5 years ago

Is this always true? Didn't @bzbarsky just say that Firefox doesn't do this?

FWIW, what Boris said is that Firefox will do a single load + parse, but it will expose two separate CSSStyleSheet objects, and if you mutate one we'll copy-on-write. So nothing that should be observable to authors.

justinfagnani commented 5 years ago

Not doing a fresh request for each @import of the same file is observable.

emilio commented 5 years ago

I meant observable from JS / the CSS OM. Of course it's observable from the server that doesn't get the request if that's what you mean.

dandclark commented 5 years ago

Playing around with some simple test pages, I’ve observed that in Chrome, Edgeium, and Firefox, duplicate @imports do not seem to cause duplicate fetches in practice. This is regardless of whether the @imports come from the same stylesheet import tree or from trees originating from separate <link> or <style> elements. So I’m not sure that overhead from extra Fetches is much of a concern in practice. The performance difference would seem to be limited to the creation of the duplicate CSSStyleSheet objects.

I mainly used this page to make these observations, in case anyone else wants to try it out. It has imports leading to “2a.css” from a few different paths, but Fiddler only observes one fetch of the file per page load.

On the other hand the Polymer customer use case is interesting. This may end up being something that we have to get into the hands of customers via a prototype to get some early feedback. I’m still concerned that the introduction of one-way links in the CSSOM tree structure could lead to awkwardness but at this point I can’t back that up with anything concrete.

argyleink commented 5 years ago

would love to see some future CSS feature that allows CSS modules to be used more similar to how JS modules are used, where all dependencies can be explicitly declared and applied only once.

^ that's aligned with what I'd like to see (and hear) as well. JS's module system went through many gauntlets to get where it's at, I'd like to see CSS follow that arterial path. In my opinion, option 3 fits more closely to the dependency graph we've grown to appreciate and may receive less criticism from the community because of this alignment.

Whichever helps beginners and advanced folks "think less" about how loading works so they can focus on their task and let the platform handle the implementation intricacies. Dependency graphs are working, let's work towards CSS having a robust graph as well?

domenic commented 5 years ago

It seems that a lot of folks in this thread are interested in a new type of imports for CSS, which work differently than @import today. I think that feature needs to be designed separately, with the help of the CSSWG.

As such, I don't think we should proceed with CSS modules until those discussions get figured out, as it seems like our original plan of "this will be easy! Just expose a CSSStyleSheet object!" will not meet community expectations. We need a more involved design process.

justinfagnani commented 5 years ago

I agree that this part should have some deliberate design, even if it's disappointing to see a delay in what I think is a very critical feature for modernizing web development.

There is a path that's forward-compatible with either outcome of the @import discussion, which is to disallow @import for now, much like CSSStyleSheet#replaceSync does. At worst, this pushes dependencies between CSS modules into JS module wrappers, which is what would happen if @import in CSS modules retained non-module CSS @import semantics. That could be undone if/when @import is supported in the future.

Note that delaying CSS modules would likely delay HTML modules too, given the current HTML modules proposal's dependency on them.

But given the need to design @import carefully, how can we move that discussion forward? Is this issue/repo even the right place? Should it be in the CSSWG? Who needs to be involved? cc @tabatkins.

dandclark commented 5 years ago

I'm in favor of the forward-compatible v1 that bans @import to allow forward progress on both CSS and HTML Modules while the discussion of a potential new @import model for v2 continues in parallel.

rniwa commented 5 years ago

I'm in favor of the forward-compatible v1 that bans @import to allow forward progress on both CSS and HTML Modules while the discussion of a potential new @import model for v2 continues in parallel.

As in throw when there is @import? Silently ignoring @import is probably not going to be forward compatible as @import start working in the future could cause problems.

dandclark commented 5 years ago

As in throw when there is @import? Silently ignoring @import is probably not going to be forward compatible as @import start working in the future could cause problems.

Yes, throwing like CSSStyleSheet.replaceSync:

  1. If rules contains one or more @import rules, throw a "NotAllowedError" DOMException.
chrishtr commented 5 years ago

FWIW, I think a v1 that throws on imports is a fine starting point. From reading the discussion in this issue, I also think that option 3 (@imported CSS is considered another module in the graph) is the best way to go, for reasons of performance: avoiding duplicated work, improved caching, and support for tooling to package and optimize sites statically.

I also think @dandclark is right that the issue discussed regarding dependencies of sheets is not really affected by this particular proposal, because the loading of the modules has to do with the network, as opposed to the cascade; the cascade is affected by where these CSSStyleSheet objects are placed by script, which is controllable and deterministic by the developer.

Lonniebiz commented 5 years ago

So, what is being proposed is just syntactic-sugar for for this?:

// Filename: styles.mjs

const style = `:host
{
    background-color: white;
    color: red;
}`;

const styles = new CSSStyleSheet();
styles.replace(style);
export { styles };
import { styles } from './styles.mjs'

class MyElement extends HTMLElement {
  constructor() {
    this.attachShadow({mode: open});
    this.shadowRoot.adoptedStyleSheets = [styles];
  }
}
dandclark commented 5 years ago

I've posted an Explainer doc for the @import-less CSS Modules V1.

We're wrapping up JSON modules and are ramping up to implement CSS Modules in Blink (most of the groundwork has already been laid with Synthetic Modules). I don't see other implementer interest concretely stated in this thread -- @annevk , @rniwa , thoughts on moving forward with the V1?

dfabulich commented 5 years ago

I don't know where to appropriately have this conversation, but I'm very worried about the migration path to this thing from the existing technology called "CSS Modules." https://github.com/css-modules/css-modules

To differentiate between the proposal here and the existing "CSS Modules" thing, I guess I'll call the existing thing "ICSS Modules," since (as an implementation detail) they compile to so-called Interoperable CSS files.

ICSS modules and CSS Modules V1 sound identical (they're both just called "CSS Modules") but they behave completely differently.

Then there's the naming conflict. Imagine trying to Google for this: "How do I port my code from CSS modules to CSS modules?"

If I may be so bold as to speak for the many, many developers who never use Shadow DOM and never plan to start using it, it seems like this proposal is just going to make our lives worse.

matthewp commented 5 years ago

@dfabulich The naming collision is unfortunate but what do you propose be done about it? In this issue "CSS Modules" is not a marketing phrase but rather a description of the feature. The same confusion exists for JavaScript modules which had a pre-existing meaning before import/export, and now there's also work being done on JSON modules.

This case is a little more confusing since CSS Modules is a project name. If we call this CSS modules lower-case does that help a little? Otherwise I'm not sure what can really be done; generic names are generic.

dfabulich commented 5 years ago

For the naming conflict in particular, perhaps we can pick a synonymous name? Here are a few suggestions:

  1. Stylesheet modules
  2. ES stylesheet modules
  3. Native stylesheet modules

I recognize that "CSS" stands for "Cascading Style Sheets" and so the semantic confusion would remain. (Honestly any proposal whose syntax begins import 'styles.css' will be confusing in that regard.) But I think picking a better syntactic name will help people to Google for the right solution when they need it.

dfabulich commented 5 years ago

To add a few more constructive comments, I would wish that this specification would be feature equivalent to ICSS modules, i.e. if I try to port from ICSS modules to "native stylesheet modules" (or whatever we call it), I wouldn't get "stuck" on major missing features.

  1. Work out a server-side polyfill library that supports scoping for CSS modules without client-side JS with solid bundling support. I anticipate that this will be hard, at least as hard as figuring out what to do with @import, and that the problems that the library implementor will encounter will/would inform even this specification as well as whatever CSS Modules V2 would look like.
  2. That may be too much to ask, but as a compromise, at least work out a polyfill intended for use with SSR that could render scoped CSS modules with an extremely tiny JS payload in the <head> of the document.
  3. Provide a way to import (and tree-shake!) individual classes/rules from a big stylesheet. That will be especially important if, in V1, users will have no way to @import, because then I'll have to pre-resolve the @imports into a big giant CSS file; including only the rules I actually need will be way more important.
web-padawan commented 5 years ago

IMO, that naming conflict should be resolved on the library side.

There is a well known case with ES2015+ related to certain Array methods names, changed because of Prototype.js reserved the originally suggested names, so that implementing them natively could break the thousands of sites.

Thankfully, this time we are talking about rebranding for a CSS authoring library. There are hundreds of those "tools-that-generate-unique-class-names-or-inline-styles" in React ecosystem. Why should we care about them?

I mean, if we give up on the name today, it might lead to consequences in future.

PS: this proposal is not tightly coupled with Shadow DOM, so let's not expand anyone's objections against using it here.

justinfagnani commented 5 years ago

@dfabulich I think the items in your constructive comments are laudable, but I'm optimistic we'll get there just fine starting from basic capabilities first.

Any polyfill for this feature, just like with JS modules, is going to essentially require a build step (excluding build-steps in the browser). There's a very simple transform of the importer and CSS file to make this work, but presumably support for the standard semantics will be added to tools that do more on top, like bundling and tree-shaking.

For importing individual rules see my CSS References proposal: https://github.com/w3c/csswg-drafts/issues/3714 Again, with this one I expect that tools will adapt to optimize on top of the standard semantics.

BTW, for tree-shaking, CSS Modules already give us a huge leap forward: by enabling finer-grained and explicit dependency CSS, we can use the native tree-shaker - the browser doesn't load modules that aren't imported.

justinfagnani commented 5 years ago

As for the name, I'm not sure what else you would reasonably call this feature. We have JS modules, JSON modules, CSS modules and HTML modules.

When I refer to ECMAScript modules I say just "JS modules" and hope that other possible ambiguous meanings will fade away over time and when we need to be specific we use "CommonJS modules", "AMD", etc. I think that's slowly happening, and hope the same will for CSS modules.

dfabulich commented 5 years ago

There's a very simple transform of the importer and CSS file to make this work

I don't think that's right in this case; the build will need to provide a server-side runtime implementation of CSSStyleSheet, which looks non-trivial to me. The build-time bundler will furthermore need a way to transform constructed CSSStyleSheet objects into bundled CSS files and scope them without client-side JS or Shadow DOM. (How, exactly?)

It is precisely my concern that you think this should be pretty trivial, but I claim that it's really really not, and that's why we should stop and think about this for another minute.

BTW, for tree-shaking, CSS Modules already give us a huge leap forward: by enabling finer-grained and explicit dependency CSS, we can use the native tree-shaker - the browser doesn't load modules that aren't imported.

But by dropping @import, it'll be a big step backward in practice, as the bundler will have to flatten all @imports together and let the browser download all of the imported rules, whether they're needed or not.

As for the name, I'm not sure what else you would reasonably call this feature

"Stylesheet modules" is short and sweet. I provided two other names as well.

justinfagnani commented 5 years ago

It is precisely my concern that you think this should be pretty trivial, but I claim that it's really really not

@dfabulich we have a lot of experience building polyfills for these features, including Shadow DOM with and without style scoping, CSS custom variables, JS and HTML modules, and a few prototypes for Constructible Stylesheets. We also have a limited version of the transform (for slightly different semantics) in use at Google.

I think we have a very good idea of what this entails, and the transform for CSS modules is quite simple, especially compared to the polyfills we've build before.

I'm a firm believer in layering for polyfills, so the transform would be based on Constructible Stylesheets so that 1) we have optimal perf in Constructible Stylesheets supporting browsers 2) can work on-top of any Constructible Stylesheets polyfill, and 3) we enable all the use-cases like adoptedStylesheets.

And like most polyfills, there will likely be multiple competing implementations and better techniques will be discovered and used if they exist. I advocate for new APIs to consider polyfill-ability, and I'm very confident that this proposal is easily and efficiently polyfillable by a build-time tool.

Dropping @import from v1 allows us to start getting the tools ecosystems to add support while the decision is worked on.

as the bundler will have to flatten all @imports together and let the browser download all of the imported rules, whether they're needed or not.

You can easily construct @import supporting transforms that do not duplicate dependencies.

justinfagnani commented 5 years ago

Here's an example Rollup transform: https://gist.github.com/samthor/ee78b434b0f9aa525c5d235979b830aa

A transform can go off-spec with a cooperating Constructible Stylesheets polyfill and support @import by lifting @imports into the JS module. I would suggest that tools don't do that though, and wait for the spec to evolve.

dfabulich commented 5 years ago

That transform requires client-side JS to work. It's transforming declarative CSS into imperative client-side JS. So, sure, that is a trivial transform, but that's not the polyfill I'm asking about.

ICSS modules running on the server side can generate scoped HTML + CSS with no client-side JS. Is that polyfill even possible under the current proposal?

matthewp commented 5 years ago

@dfabulich This proposal does not attempt to solve all of the same problems that CSS Modules solve, only the problem of being able to import CSS from JavaScript. In the future other specs could solve some of the other things that CSS Modules give you. It's common to release spec features iteratively rather than pack many features into 1 proposal because the latter is less likely to gain consensus.

dfabulich commented 5 years ago

That's true, but this feature is clobbering an existing userland solution that solves the problem better than the proposal on the table.

Most features aren't clobbering any existing userland solution; under the extensible web, most new features add some totally new capabilities, and if V1 hasn't added enough capabilities, maybe we can add them in later, but having partial capabilities earlier is worth it. Not so in this case.

This feature violates the norm to "first, do no harm."

If we're going to introduce a naming conflict, with syntax that's confusingly similar to a widely loved existing userland transpiled solution, then it had better be worth it. It shouldn't just be an 80% solution compared to userland; it ought to be actually better than userland if we're going to incur these costs on the web community's behalf. People using the old userland thing should say "It's a pain that I have to upgrade, but I'm glad they introduced this new thing; this is way better than our hacked up solution. Let's abandon the userland approach in favor of the new standard."

And if it's not worth it in the current form, but we still want to move ahead with it anyway, then it ought to at least be forward compatible with a solution that actually is better than userland. We ought to know what that solution could look like, making sure that we won't have to break backwards compat when we all finally adopt V2 or V3 which actually solves these problems.

I'm sure that a good solution for @import can be devised at some point, but I'm skeptical that even a polyfill with no client-side JS can be prototyped for CSS Modules V1.

First, do no harm.

justinfagnani commented 5 years ago

This feature doesn't interfere with the userland solution in any way. The userland solutions are implemented as custom transforms that will absolutely still work with no changes whatsoever, because the CSS module import doesn't reach the browser.

Those transforms can carry on from now till eternity, or they can be updated to perform a new subset of their transforms on the CSS contents and use CSS modules for loading and parsing. Systems that provide more exports from CSS than this proposal (like classes) can still do so by generating the necessary JS.

The example taken from https://github.com/css-modules/css-modules:

style.css:

.className {
  color: green;
}

app.js:

import styles from "./style.css";
// import { className } from "./style.css";

element.innerHTML = '<div class="' + styles.className + '">';

can be transformed to:

style.css:

.a /* renamed from .className */ {
  color: green;
}

style.css.js:

import {style} from './style.js';
document. adoptedStyleSheets = [...document. adoptedStyleSheets, style];
const classes = {
  className: 'a',
};
export default classes;
export const className = classes.a;

app.js:

import styles from "./style.css.js";
// import { className } from "./style.css.js";

element.innerHTML = '<div class="' + styles.className + '">';

So this preserves existing semantics and leverages native loading. If they wanted to update the transform, say to be usable with shadow DOM, they could have a version that exports the stylesheet and doesn't automatically apply it to the document.

There are a ton of options on how the basic semantics of importing a stylesheet can be used and extended for all kinds of cases and bridged to existing userland systems. It'd be more useful if general claims that this proposal is in conflict with such systems, and especially that it might "do harm", were accompanied with more specific examples.

dfabulich commented 5 years ago

The harm is the naming conflict and the syntax conflict.

Syntax conflict: What does import styles from 'styles.css' do? I know what it does now because that's the syntax for CSS modules. In the future, it will also be the syntax for CSS modules, but it will be difficult to know which CSS module system is in use. Can we mix and match CSS modules with CSS modules? Does import styles from 'style.css' work on the server side?

These are settled questions today, but not when this feature ships. It will create significant confusion and incompatibilities between two identically named CSS module systems. That's harm.

The web community still hasn't finished absorbing the backwards compatibility problems introduced by ES modules. The old ways still work! But they look identical to the new ways, and they don't work together. That harmed the community, in hindsight. This does something similar.

I recognize that you can't make this particular omelette without breaking a few eggs. But in this case, we already have a delicious omelette. The new omelette has to be better than the omelette we already have.

justinfagnani commented 5 years ago

import styles from './styles.css'; is not yet standardized, so you only know what it does now if you're using a particular tool. There are a number that utilize that same syntax with slightly different behavior:

These are just a few of the tools, but they do have some commonalities:

All-in-all I think this capability will be a huge boon to the userland solutions which will now have a standardized compile target. Packages can use a tool's proprietary semantics, but transform to standard semantics before publishing. This will unlock many packages to publish standard JS modules to npm.

Jamesernator commented 5 years ago

So something I've been playing around with is creating a transform system that is a superset of this proposal with the things I mentioned here supported as well.

In particular I hope to be able to solve ICSS modules use cases (but not necessarily be syntax compatible) while also supporting Shadow DOM well and keeping the door open for future extension.

In particular I aim to have imports from both CSS and JS of values, keyframes, selectors, custom property names and custom names (e.g. for paint($name)) working within a few weeks.

I haven't decided exactly how selectors will work, but I'm leaning towards treating them more like classes than rulesets, so that generic selectors work e.g.:

/* metrics.css */

$gridBaseline: 8px;
/* mixin.css */

@import "./metrics.css" {
  names: $gridBaseline;
}

$mixin {
  background-color: red;
  color: black;
}

$mixin > p + p {
  margin-block-start: calc($gridBaseline * 2);
}

$mixin > h1 {
  margin-block-start: calc($gridBaseline * 4);
}

$large {
  @extends $mixin;
  font-size: 1.2em;
}
/* myComponent.css */
@import "/path/to/mixin.css" {
  names: $mixin;
}

#main {
  @extends $mixin;
}
/* myComponent.js */
import styles from './styles.css';
import { large as largeStyles } from '/path/to/mixin.css';

/* ... */
this.shadowRoot.adoptedStyleSheets = [styles];

/* ... onChange */

if (this.getAttribute('size') === 'large') {
  this.shadowRoot.querySelector('#main').adoptedClasses.add(largeStyles);
}
/* ... */
dfabulich commented 5 years ago

All-in-all I think this capability will be a huge boon to the userland solutions which will now have a standardized compile target.

We already have a standardized compile target: HTML and CSS, with no client-side JS required.

• They are side-effectful. They all append a stylesheet to the main document. That's not a behavior we'd want in the JS module system where modules are by default side-effect free, unless code in them has a side-effect.

I don't understand your point here. It seems like a tautology. Modules are side-effect free by default…unless they have a side effect. These modules certainly do have side effects, and that is part of their design, just like importing a module that declares a custom element.

Your other points seem to emphasize that ICSS Modules won't stop working when this ships; I agree that they won't stop working. I never said they would. I said that the new thing would create incompatibility and confusion between two identical syntaxes with the same name ("CSS Modules"). I won't be able to mix-and-match CSS Modules with CSS Modules.

If CSS Modules V1 solved all of the same problems of ICSS modules and more, then it might be worth paying that price, but it doesn't, so it isn't. We should only clobber ICSS modules if we have a solution that is so superior to ICSS modules that it would be worth switching to it.

csvn commented 5 years ago

We already have a standardized compile target: HTML and CSS, with no client-side JS required.

@dfabulich If we want to import CSS, then no, we can't compile to CSS. The CSS has to either be wrapped in a Javascript file via e.g. default export, or use fetch to retrieve CSS text content.

// style.css
export default `
  body { /* style */ }
`;
// or
const style = await (await fetch('/style.css')).text();

After this, a style tag or Constructible StyleSheet must be created and the text inserted. This is quite a bit of extra code for a trivial example over simply using import style from '/style.css' and getting back something that is directly usable.

dfabulich commented 5 years ago

If we want to import CSS, then no, we can't compile to CSS.

But that's exactly what we do, right now. We import CSS on the server side and it compiles to CSS. It's quite nice! It's better than this proposal.

As I said, I recognize that import style from 'style.css' is inherently going to use the same syntax as ICSS modules, and so you can't make this omelette without breaking those eggs. But this proposal isn't better than the userland solution, so it's not worth the cost of creating incompatibility and confusion between two identical syntaxes with the same name.

dtruffaut commented 5 years ago

Can I do lazy-loading / code splitting using dynamic import with await import ?

Example :

document.adoptedStyleSheets = [...document.adoptedStyleSheets, await import('./style.css')];
justinfagnani commented 5 years ago

@dtruffaut yes

dfabulich commented 5 years ago

I believe it would be unwise to write it in one line like that, because ...document.adoptedStyleSheets will evaluate before the import does its fetch. If someone adds a stylesheet to adoptedStyleSheets during the import, it will be blown away when the import finishes.

Splitting it into two lines will fix it:

const style = await import('./style.css');
document.adoptedStyleSheets = [...document.adoptedStyleSheets, style];
Jamesernator commented 5 years ago

These modules certainly do have side effects, and that is part of their design, just like importing a module that declares a custom element.

Attaching global stylesheets simply doesn't work for shadow DOM, so being compatible with ICSS while supporting shadow DOM is not going to happen for any actual CSS modules implementation.

Regarding the verbosity concerns, personally I'd rather just see CSS modules be able to be included directly in HTML when dynamic removal/addition isn't actually required. e.g.:

<!-- Uses the CSS module system and caches the same sheet -->
<style src="./styles.css"></style>

<div class="someClass">
  ...
</div>
justinfagnani commented 5 years ago

@dfabulich @dtruffaut

async function go() {
  document.adoptedStyleSheets = [...document.adoptedStyleSheets, await import('./style.css')];
}

would work just fine. The inner expressions are evaluated first, yielding at the await, then the array literal, then the assignment, etc...

Edit:

I would have never have written this code if we had a mutable object, like an Array. It very obviously would have been:

async function go() {
  document.adoptedStyleSheets.push(await import('./style.css'));
}
Rich-Harris commented 5 years ago

I think @dfabulich is right:

arr = ['a'];

function resolveAfter (value, ms) {
  return new Promise(f => setTimeout(() => f(value), ms));
}

async function addB() {
  arr = [...arr, await resolveAfter('b', 100)];
}

async function addC() {
  arr = [...arr, await resolveAfter('c', 50)];
}

addB();
addC();

// later...
console.log(arr); // ['a', 'b']

This is a strong argument in favour of having an interface for adding and removing styles, rather than using an array literal, which is likely to result in some unpleasant bugs for this reason.

tabatkins commented 5 years ago

Why would you do that, rather than just getting/setting document.adoptedStylesheets in each function?

Also, note that this is a consequence of await semantics in argument position; they're a little unobvious and should generally be avoided. (Basically, all preceding arguments are eagerly evaluated before the function pauses.) If you wrote those functions as:

async function addB() {
  const newVal = await resolveAfter('c', 50);
  arr = [...arr, newVal];
}

...you'd be just fine and it would work as expected, with arr being ['a', 'c', 'b'] at the end.

tabatkins commented 5 years ago

Oh, that's the exact code Justin listed. ^_^ Dont' do that code, Justin! It's bad!

Rich-Harris commented 5 years ago

Which is the point. If Justin Fagnani's intuitions about how that code behaves are wrong, imagine what mischief the average developer will get up to!

Saying 'don't do that' isn't adequate, in my view. Good API design leads you into the pit of success; this does the opposite. The fact that it took so long in this thread before anyone noticed this issue is a foreshadowing of how many subtle race conditions will go unnoticed if this is what ends up in browsers.

justinfagnani commented 5 years ago

I didn't say that @dtruffaut 's code was free of race conditions, but that it would work. His snippet was outside of an async function, so I put it in one to make it valid. My intuition wasn't incorrect because I wasn't making a broader claim.

But now we're talking about async functions, race conditions, and adoptedStyleSheets, which are not CSS modules. CSS modules only allow you to import a stylesheet. If there are more ergonomic ways if consuming stylesheets in the future, CSS modules will naturally and seamlessly work with them. That's the benefit of a targeted and incremental proposal, rather than something that tries to duplicate features and semantics from much larger-in-scope userland solutions out of the gate.

Rich-Harris commented 5 years ago

Well, we have a different definition of 'it would work'. Regardless, where is the appropriate place to discuss adoptedStyleSheets? I would argue it needs some more consideration.

caridy commented 5 years ago

I believe @rniwa as complained about this exact issue, or similar issues with the array. The adoptedStyleSheets API is just awkward, and counter intuitive. I hope that we can still fix that portion, and get Apple folks onboard.

justinfagnani commented 5 years ago

Can someone open another issue to discuss adoptedStyleSheets? I'm not sure where it was discussed before.