-
Notifications
You must be signed in to change notification settings - Fork 141
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
[worklets] dynamic import() #506
Comments
See whatwg/fetch#527 (comment) for issues if we decide to allow it somehow. |
I agree that it makes sense we shouldn't have |
I'm not sure what the behaviour should be, if |
If we are disallowing it, I’d say hide it. A function whose only purpose is to throw seems odd. |
It's not a function; it's syntax. It can't be removed without language changes. I'm not even sure what hiding it would mean. |
Oh didn't realize that, makes sense. |
@domenic I assume we can make it reject the promise it returns? (Depending on what data gets passed to Fetch, we could even return a network error there for all fetches made by worklets. Not sure that's ideal however as it's unlikely to be implemented that way.) |
Yeah, that's doable. |
Filed a tracking issue in crbug. |
To be clear, I don't think we should reject import() in worklets, per my position from the earlier thread on the same topic. I think all forms of importing code should work in worklets, including lazily importing. I was only commenting on the technical feasibility here. |
I wonder how worklets work with the opaque origin setup if their modules can have non-dynamic imports. Did you review that @domenic? Or maybe the opaque origin is assigned after all dependencies are fetched? The main reason to block dynamic |
IIUC, non-dynamic imports are handled as sub-resources of the owner document like |
Thanks. But the referrer would be the top-level module script's (base) URL? So yeah, if we want dynamic |
Yes. It's the same with plain ES6 modules. I'm now adding WPT tests for the referrer of descendant scripts (crbug). |
Part of the justification that it shouldn't work for me at least, is that worklet global scopes are allowed to be transient, and when they are created again they should contain all the same code as the other WGS. Allowing the dynamic import here makes it a little too easy for them to diverge. We won't be able to remove all non-deterministic apis, but this one seems easy to reason about? |
The Working Group just discussed
The full IRC log of that discussion<TabAtkins> Topic: dynamic import()<TabAtkins> github: https://github.com//issues/506 <TabAtkins> iank_: ES6 modules are a thing <TabAtkins> iank_: worklets are built on them <TabAtkins> iank_: WE have a dynamic import() now <TabAtkins> iank_: you can say import(url), returns a promise <TabAtkins> iank_: Do we want to allwo in worklets? <TabAtkins> TabAtkins: And we don't allow fetch(), so... <TabAtkins> Rossen: Motivation? <TabAtkins> iank_: Consistency with other global scopes. <TabAtkins> smfr: Can't do anything async, right? The paint has be to sync. <TabAtkins> iank_: Right. It would just be a communication channel to the server. <TabAtkins> dbaron: Woudl this expose whether you're recycling the global? <TabAtkins> iank_: Potentially. It wouldn't be intercepted by a SW. <TabAtkins> smfr: And it would mean your context is running code at a weird time. <TabAtkins> iank_: So room opinion seems to be to throw? <TabAtkins> TabAtkins: If we don't allow any other network communication... <TabAtkins> Rossen: Yeah, with all the worklets, just makes more sense. <TabAtkins> iank_: So the proposed resolution is that dynamic import becomes a no-op that returns a rejected promise. <TabAtkins> RESOLVED: dynamic import() in worklets is a no-op that returns a rejected promise (or equivalent after passing thru JS people) |
I don't think we should be piecemeal disabling parts of the JS language. It should be principled, e.g. adding a switch that disables dynamic import and Math.random() and Date.now()/new Date()/etc. at the same time. |
It is principled, no network APIs. |
Then static import should be disallowed. |
Why? Those are all resolved before execution. |
They're still network APIs, so it seems the line is not very principled? |
Anne is right. This keeps inline with "no communication to the outside world" once you've added the scripts, i.e. no networking apis. |
What error code does |
If there is no objection, I'm going to make |
Apologies for not replying sooner. I think |
@annevk Thank you! |
All networking APIs including dynamic import() are disallowed on WorkletGlobalScope. This CL makes dynamic import() always reject a promise with TypeError[1] on WorkletGlobalScope. [1] w3c/css-houdini-drafts#506 (comment) Bug: 782538 Change-Id: Ieb94a4502b0ba35ff9b56e51c8fc34467ab96d0c Reviewed-on: https://chromium-review.googlesource.com/790059 Commit-Queue: Hiroki Nakagawa <[email protected]> Reviewed-by: Ian Kilpatrick <[email protected]> Reviewed-by: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#521295}
I thought we already had an issue for this somewhere. If worklets cannot have
fetch()
, I don't think it makes sense for them to have dynamicimport()
. That would cause the same kind of execution problems.cc @domenic @nhiroki @bfgeek
The text was updated successfully, but these errors were encountered: