-
Notifications
You must be signed in to change notification settings - Fork 142
[worklets] First pass of fixing import for worklets. #251
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
Conversation
@@ -262,49 +265,83 @@ is empty; it is populated when the user agent chooses to create its {{WorkletGlo | |||
A {{Worklet}} has a list of the <dfn>worklet's resolved module URLs</dfn>. Initially this list is | |||
empty; it is populated when module scripts resolved. | |||
|
|||
A {{Worklet}} has a <dfn>fetched modules map</dfn>. This is a map of <a>fetch</a> requests to values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "module responses map"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want this to be a map of request URLs -> responses. Because "fetch a module script tree" will give you a new request every time, so you can't use the requests as cache keys. This also allows you to eliminate the "worklet's list of resolved module URLs"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "module responses map". I've still got "worklet's list of resolved module URLs" at the moment as the load ordering when initializing the worklet is still important.
Could change this to just an "ordered map"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, that sounds good to me. In JS all maps are ordered so that was natural for me :). Maybe say things like "the list of keys of the module responses map, in insertion order".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Core of the algorithm looks pretty good. Some minor suggestions leading up to it. I'll get working on that patch to HTML now. |
This allows worklets to use their own cache for module fetching, as discussed in w3c/css-houdini-drafts#229 and w3c/css-houdini-drafts#251.
<a>API base URL</a> specified by the <a>entry settings object</a> when the method was | ||
invoked. | ||
2. Let |resolvedModuleURL| be the result of <a>parsing</a> the |moduleURL| argument relative to | ||
the <a>settings object</a> of <b>this</b>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want relevant settings object of this
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@bzbarsky let's talk about settings objects. I'm starting from the assumption we want to stick to current or relevant. The code in question is: window1.Worklet.prototype.import.call(window2.someWorklet, url); URL resolution: Current means this resolves relative to Picking the "outside settings object": The question is whether the fetch client for the module requests is |
This allows worklets to use their own cache for module fetching, as discussed in w3c/css-houdini-drafts#229 and w3c/css-houdini-drafts#251.
I ran a test and it appears that the Worker constructor's specced choice of incumbent does not match browsers, which all use current. (At least, Firefox and Chrome do; Edge doesn't seem to send referrer headers so I can't test, and I haven't tested Safari yet.) See https://settings-object-worker-client-ckrcsrtdqr.now.sh/entry.html for a test. So that further implies current is correct for picking the outside settings object. I'll update Workers tomorrow in HTML. I'll also test URL resolution in the Worker constructor tomorrow. |
There's a fundamental difference between constructors and instance methods: with constructors, the current settings object is the relevant settings object of the thing being constructed. With instance methods, the two are not necessarily related. I think our general rule as stated is probably wrong. It's the general rule in ES for lack of anything else because ES doesn't have a concept of "relevant", but in practice ES has all sorts of hooks for overriding the behavior for instance methods (using Symbol.species and the like). The current global mostly gets used in observable ways in ES as a fallback when other, saner, things have failed. In this particular case, it seems to me that relative URL resolution for a worklet would naturally be a property of the worklet itself, not of the function you happened to call to trigger the load. Or put another way, if you import two things into the same worklet but using |
(Sorry for cluttering up your pull request, @bfgeek!)
Hmm. I don't really agree, because I think the idea of objects having realms is a bit unnatural. As a developer I think if anything I'd expect the object's realm to be that detected via its In other words, putting on my naive developer hat, I'd conceptualize Worklet.prototype.import = function (url) {
const resolvedURL = new URL(url, location.href).href; // (1)
// ...
fetch(resolvedURL, ...).then(...); // (2)
} Here at both (1) and (2) I am implicitly using "current". I'd have no way to get "relevant" from JS code; it's fairly unnatural to think of doing so. (The closest I could come is
In contrast, to me it seems that the function that I happened to call to trigger the load is the one doing URL resolution, so it should be a property of the function, since that's what's actually executing the load.
When you make it concrete like this, I can't say I have any particular expectations. Both seem equally weird to me. You'd be forcing me as a developer to confront the question of "what does a relative URL really mean in a multi-global context," and I don't have a good intuitive answer. I can only fall back to more principled arguments like the above. That said, I'm OK changing our position here. If you find my above arguments unpersuasive, then I'd like to get the resulting advice written up into HTML's multi-globals section. I can't tell if you're advocating for a more general "relevant by default, current when nothing else makes sense" position, or specifically for saying that URL resolution should use a relevant object. (We should probably also include a note that in constructors, relevant = current.) |
I honestly don't see why. Especially for IDL objects, which are explicitly specified as "having realms". But more to the point, it's not whether objects "have realms" or not but whether they are associated with some particular state that may happen to be associated with a Realm, or may not be. In this case, a base URL, or a source of such.
That's obviously not workable in general because the proto chain may not lead to anywhere.... And yes, I know that some ES builtin objects work if you null out their proto and some start misbehaving badly, basically depending on whether people tried to make things work that way or not. It's quite inconsistent and a bit annoying, really.
That's fair, except that's not how other things that do base URLs in the platform work. Ignoring for the moment that
You can think of "relevant" as a symbol-named property on the Or heck, a value stored in a weakmap.
Again, that's not how anything works in the web platform right now. In practice the model is that all the API functions call some sort of internal functions that do the actual work. This is in some sense an accident of history (in that the internal functions are implemented in C++ and whatnot), and we can obviously have an argument about how important consistency is. But that's how things work right now, and trying to change the model piecemeal is not very appealing to me, precisely because it means we throw consistency out the window forever.
I'm not sure whether I am. I'd have to see what things do in practice, and in particular what uses people actually have for settings objects. We have some obvious uses: base URLs (uses entry or relevant), initial prototypes for newly created objects (largely uses relevant in browsers, but uses current in some cases when the creation is done by the binding layer itself; think rejected promises when the It seems to me that such a survey is well worth doing before deciding what our general advice should be, because we should be valuing compatibility and consistency over theoretical purity...
As usual, "sort of". The relevant settings of the object that gets created == current. But the relevant settings of Note that I'm on vacation for the next week, so responses will be a bit laggy, sorry. |
This allows worklets to use their own cache for module fetching, as discussed in w3c/css-houdini-drafts#229 and w3c/css-houdini-drafts#251.
So I think we should probably go with relevant here since @bzbarsky seems to strongly prefer that. I don't want to block this getting merged. |
Thanks for the review all! Changed outsideSettings to relevant settings object of this. |
This allows worklets to use their own cache for module fetching, as discussed in w3c/css-houdini-drafts#229 and w3c/css-houdini-drafts#251.
@domenic A first pass of fixing worklets import using a shared cache. Wording for the cache is very similar to the module map in html, and seems ok?
Not happy with "fetched module map" naming.
cc/ @bzbarsky @annevk