-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Integration with import-attributes #5640
Comments
Let's also track the HTML integration PR for module attributes in this issue. A big issue with sending an |
I think the type should be part of the module map cache key to solve those issues. |
@domenic Could you say why? If it's important to send different HTTP headers, which should we send? (Note that the previous JSON module patch didn't send different HTTP headers AFAIK, and the module type was not part of the cache key.) |
To be concrete, the conflicting intuition from JavaScript developers about making the type part of the cache key would be that a module specifier points to a unique module; that it's not "cloned" just because different attributes are used. While this rules out a lot of potential use cases for module attributes, I thought it would be consistent with the requirements for The current proposal draft contains a MUST-style requirement that modules not be cloned, but TC39 understands that hosts might willfully violate this requirement (as HTML has willfully violated the JS spec in the past), and is open to reconsidering this requirement in the future. See this section of the import attributes README for details. |
fwiw, we don't make credentials mode part of the cache key, even though it changes the shape of the request. |
Different |
I do like the idea of being able to import the same module twice with different types. |
OK, then maybe it's OK to "race" here and let the first thing that loads the module specifier determine the shape of the request. In this case, how should the request to the server be modified based on the module type?
I don't understand this assertion. I'm still not a fan of import specifier microsyntax, for reasons discussed elsewhere (e.g., tc39/proposal-import-attributes#11).
If we want module attributes that can transform the interpretation of a module, while others checked things about the module (like the MIME type), what if we supported two different kinds of attributes? It could be like this: import { foo } from "specifer" if { checkKey: "value" } with { transformKey: "value2" }; Since some module attributes are checks and some are transforms, making them syntactically separated would let developers understand which parts form part of the cache key/extended specifier and which parts don't. Based on previous conversations, I thought cc @syg |
|
I also wanted to mention: about @jakearchibald 's suggestion to allow you to import a module |
Concretely: if |
This comment has been minimized.
This comment has been minimized.
What's the problem with using a separate attribute key for this case of reinterpretation as opposed to MIME type checking? We several requests from JavaScript developers who were worried about developer confusion coming from a single module specifier leading to multiple module identities, so I wanted to at least give predictable behavior to |
I don't think adding more complexity is the right answer here. Asking developers to figure and memorize the mapping between module type and |
Type does not just check the MIME type. It changes the interpretation. If you interpret |
To clarify, the semantics that we were proposing was that modules would remain strict about their MIME type: |
Like Daniel, I also thought |
Sorry, I have hidden my previous comment to avoid confusion. |
@domenic could you expand on your point of view on how it's not a fatal assert? I've read through this issue and the PR and I'm still not sure why you are proposing type as an additional key. |
Sure. There are several considerations here. They all revolve around the fact that the module map is crucial to the fetching infrastructure.
|
Thanks. That addresses the "what", but it's still not clear to me "why". Why would the same specifier be used for a different type of resource? Are you anticipating/envisioning client-side-driven content negotiation? Are there proposals to change the |
It's not really sensible for authors to use different types on the same specifier, but the important thing is that if they do, they should get sensible behavior (and not, e.g., enable poisoning of the module graph for other parts of the app). |
How can they use different types? Why are they more likely to poison the module graph this way than through referrer poisoning? |
For example, if import "https://example.com/library-2.mjs" if { type: "json" }; and we cached the resulting failure (like we currently cache all failures), that would poison the module graph so that import "https://example.com/library-2.mjs" if { type: "javascript" }; There are multiple ways to fix this, but by far the most natural way is to make the type part of the cache key, so that we cache a failure for (This illustrates just one of my three "observable behavior" sub-bullets, FWIW; similar examples can be constructed for the other two, plus probably other scenarios.) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
(Apologies for the length of this post.) In the September 2020 TC39 meeting, TC39 reached consensus on advancing Import Assertions to Stage 3 on the basis of permitting HTML to do what it does in #5883: use the assertion as part of the cache key, but not reinterpret the module on the basis of this assertion. I think it's great that TC39 expressed flexibility and technical deference to work together here. TC39 also left up to hosts the ability to decide how to treat assertions that it doesn't know how to handle (with many in the community expressing a preference for ignoring them, so the assertions "pass", but no requirement made on hosts). The HTML PR at #5883 takes a middle path: It treates the When thinking about this problem, we previously got a bit caught up on the details of SRI (which wouldn't be the best application, as it would be better to transmit out-of-band). However, other predicates
In general, I think any assertion at all would want to have the property that, if you independently import the module without the assertion, you're still talking about the same module (maybe module "server shenanigans"). For the case of However, it's not clear to me how to extend this strategy to any of the above assertions. One way would be for "normalization" to remove them. Is this the intended path? If unrecognized assertions were permitted and included as part of the module map key "unnormalized", then the strategy of "normalizing" certain recognized import assertions would lead to scenarios with two different module copies in older browsers, and the same module in newer browsers. This behavior doesn't match the transparent upgrade that ignoring unrecognized attributes is intended to provide. So it's good that this PR, at least, does ban unrecognized assertion keys and types, to preserve the ability to exclude future assertion keys from the module map key. I'd prefer to address this issue by having HTML not treat import assertions as part of the cache key, as in an earlier version of the integration PR. I still don't understand the technical issue with these semantics. It seems to me like a logical error to treat assertions as part of the cache key, as they are just checking something about the module, not logically referring to a different one, and the above examples make this mismatch concrete. This change could enable us to ignore, rather than reject, unrecognized assertions. |
I think @domenic's concern would still apply. With not treating asserts as part of the cache key you end up with race conditions if you or your large software project is not consistent with assertions. I'm not entirely sure I understand the counter argument. Why wouldn't a browser either completely ignore or always fail on unsupported assertions? That seems more logical to me than trying to do something with them. |
@annevk I'm having trouble understanding the race condition issue. I was imagining that each time the module is imported, the assertion would be checked (possibly with evaluation of this assertion being cached inside the module itself, if we add non-trivial assertions). I think @dandclark addressed all the race conditions in the specification mechanics. If you could give an example of a scenario where such a race condition would occur, then it would help me understand better.
My suggestion is that we could adopt the behavior of completely ignoring the unsupported assertions only if they're excluded from the module map key. If they're included in the module map key, then "ignoring" them would present forward-compatibility issues if they're later normalized, so in this case, it's best to do as the current PR does and always fail on them. (I'm not sure what you mean by doing something with them; maybe my writing above was unclear.) |
I guess that model still confuses me. I would expect that once something is in the module map there would not be a way to get something else out of the same entry. But yeah, you're right that it could work without race conditions. I was thinking that either you ignore it completely, or you support, normalize, and use it as part of the key. |
The only things you'd get out would be the single module or an error. Would this be confusing?
I'd be in support of this change. The current PR instead causes an error for unknown modules (partly since I was under the impression that, with @domenic 's requirements, we'd have to treat all assertions as part of the key). We could change this to ignore and permit unknown assertions. |
Is it not still an option to reject unknown assertions (by failing the import) without committing to whether or not potential future assertions should be part of the module cache key? In #5883 (and #5658), unknown assertions are currently rejected at the very beginning of fetch a single module script, before anything is fetched or added to the module map. So there are no side effects of a rejection at this point other than failing the module graph, and this behavior does not box us in to any decisions about whether future potential assertions are or are not part of the module map cache key. And since the rejection occurs before fetching/caching, I don't see what race condition could occur. I'm not strictly against ignoring unknown assertions, but rejecting them seems more future proof and safer. If some new security-focused assertion is introduced, I would expect developers to prefer such assertions to fail on browsers where they are not yet supported, rather than be ignored silently. |
@dandclark failing on unknown assertions makes it impossible to ship code that can work in both pre- and post- environments (ie, an env that doesn't yet understand a new assertion, plus an env that does). The most future-proof and safe thing is absolutely to ignore them, otherwise the adoption penalty for new assertions is too high. |
Thanks @ljharb, that makes sense. I changed the PRs for import assertions + JSON modules and import assertions alone to completely ignore unknown assertions. Unrecognized values of the |
@dandclark Thanks for these updates. With them, #5883 looks good to me. I don't think we should land #5658 because it overlaps with the TC39 JSON modules effort. |
Hmm, currently #5883 (and #5658) checks for unknown values of the This is getting into the weeds a bit, not sure if this discussion would be better off in #5883. |
@dandclark I'm inclined to just agree to whatever's simpler/more consistent, but out of curiosity: Can you give an example of an observable difference from this change? Is it about which error class is thrown when the module graph fails to load, or does this affect timing? |
In terms of complexity I think both options are pretty similar. One observable change would be whether or not I think another observable difference would be whether, for the following script foo.js,
If notARealType is checked when In both cases I prefer the behavior of checking in |
Actually if the check is removed from |
I kind of talked myself into it, so I made the change (c225d56). I can back it out if I'm alone in thinking this is the right approach. |
This change extends the JavaScript module system integration to include CSS module scripts, in addition to JavaScript module scripts. These allow web developers to load CSS into a component definition in a manner that interacts seamlessly with other module script types. Explainer document: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md Closes WICG/webcomponents#759. Part of #4321. This change includes the integration for the import assertions proposal (https://github.com/tc39/proposal-import-assertions/), thus closing #5640 and closing #5883 by superseding it.
This change extends the JavaScript module system integration to include CSS module scripts, in addition to JavaScript module scripts. These allow web developers to load CSS into a component definition in a manner that interacts seamlessly with other module script types. Explainer document: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md Closes WICG/webcomponents#759. Part of whatwg#4321. This change includes the integration for the import assertions proposal (https://github.com/tc39/proposal-import-assertions/), thus closing whatwg#5640 and closing whatwg#5883 by superseding it.
The proposal https://github.com/tc39/proposal-import-attributes, currently stage 2, provides a way for developers to add attributes to the in the import, for instance:
The host will ensure that the resource is of type JSON.
In tc39/proposal-import-attributes#61 we are discussing if the host would send a header indicating the expected type to the server, I suppose that would be
Accept
. We decided that the type attribute isn't part of the module cache key.On the other hand, tc39/proposal-import-attributes#16 shows that multiple representation of a resource may exist; BinAST and JavaScript for example.
Does sending a header based on the module type or other semantics make sense to you?
The text was updated successfully, but these errors were encountered: