RESOLVED FIXED 164860
JS Modules in Workers
https://bugs.webkit.org/show_bug.cgi?id=164860
Summary JS Modules in Workers
Yusuke Suzuki
Reported 2016-11-17 01:18:38 PST
...
Attachments
Patch (27.74 KB, patch)
2020-06-19 16:47 PDT, Yusuke Suzuki
no flags
Patch (28.34 KB, patch)
2021-02-07 23:33 PST, Yusuke Suzuki
no flags
Patch (28.32 KB, patch)
2021-02-08 01:26 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Patch (29.04 KB, patch)
2021-02-08 01:52 PST, Yusuke Suzuki
no flags
Patch (66.94 KB, patch)
2021-02-09 03:04 PST, Yusuke Suzuki
no flags
Patch (97.78 KB, patch)
2021-02-09 20:26 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Patch (102.99 KB, patch)
2021-02-10 00:29 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Patch (111.39 KB, patch)
2021-02-10 00:39 PST, Yusuke Suzuki
no flags
Patch (111.46 KB, patch)
2021-02-10 13:40 PST, Yusuke Suzuki
no flags
Patch (140.58 KB, patch)
2021-02-10 16:46 PST, Yusuke Suzuki
no flags
Patch (145.20 KB, patch)
2021-02-10 18:11 PST, Yusuke Suzuki
no flags
Patch (171.03 KB, patch)
2021-02-11 11:24 PST, Yusuke Suzuki
no flags
Patch (224.68 KB, patch)
2021-02-11 14:55 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Patch (224.52 KB, patch)
2021-02-11 15:13 PST, Yusuke Suzuki
no flags
Patch (230.10 KB, patch)
2021-02-11 20:18 PST, Yusuke Suzuki
no flags
Patch (230.11 KB, patch)
2021-02-11 20:26 PST, Yusuke Suzuki
no flags
Patch (230.21 KB, patch)
2021-02-11 22:52 PST, Yusuke Suzuki
no flags
Patch (232.99 KB, patch)
2021-02-12 01:12 PST, Yusuke Suzuki
no flags
Patch (232.99 KB, patch)
2021-02-12 22:57 PST, Yusuke Suzuki
no flags
Patch (233.37 KB, patch)
2021-02-17 01:58 PST, Yusuke Suzuki
no flags
Patch (241.54 KB, patch)
2021-02-18 23:02 PST, Yusuke Suzuki
no flags
Patch (244.69 KB, patch)
2021-02-18 23:36 PST, Yusuke Suzuki
no flags
Patch (245.29 KB, patch)
2021-02-19 11:26 PST, Yusuke Suzuki
saam: review+
Advanced use case (798.71 KB, image/png)
2021-04-24 02:09 PDT, Tobias Uhlig
no flags
Saleh Abdel Motaal
Comment 1 2018-07-07 05:59:18 PDT
It seems that support `new Worker(…, {type: "module"})` is limited or missing. In a project that uses only pure ES modules served using a very basic express server (ie express.static) over http://127.0.0.1:8080 or http://localhost:8080 a worker module with static import statements: ``` import {…} from '…' ``` Throws: SyntaxError: Unexpected token '{'. import call expects exactly one argument. It should be noted that those static import statements are synthesized using rollup and the resulting files sometimes include one or more variable declarations before the occurrence of the import statements. The same project does not throw in Chrome.
Guy Bedford
Comment 2 2019-09-11 11:47:33 PDT
Note that supporting dynamic `import()` in Web Workers can also be a first start along these lines.
Tobias Uhlig
Comment 3 2019-11-03 12:02:36 PST
Just stumbled on this ticket and am quite surprised that it did not get way more comments yet. I am crafting a web workers driven ES8 framework, so module support for workers (including dynamic imports) is one of the key features lacking in Safari. In Chrome this works already for close to a year using the experimental web plattform features and now got the Label Target-80. Without the type="module" support, applications have to use build tools like Webpack, which is slowing the web development speed down by a long shot! Please be aware, that this ticket should also include the module type for shared & service workers. I recommend to make it a high priority epic. Thanks & best regards Tobias
Tobias Uhlig
Comment 4 2019-12-20 13:06:22 PST
A quick update on the current status would be appreciated. The new framework is online now and already at 227 github stars (December 20): https://github.com/neomjs/neo Here are the online examples, which according to the specs should work in every modern browser: https://neomjs.github.io/pages/ It makes me a bit sad to see Safari falling behind on an such important topic. https://html.spec.whatwg.org/multipage/workers.html#module-worker-example Hoping you will catch up! Best regards & happy holiddays, Tobias
Radar WebKit Bug Importer
Comment 5 2020-06-14 19:44:14 PDT
Yusuke Suzuki
Comment 6 2020-06-19 16:47:17 PDT
Created attachment 402347 [details] Patch WIP
Yusuke Suzuki
Comment 7 2021-02-07 23:33:13 PST
Yusuke Suzuki
Comment 8 2021-02-08 01:26:54 PST
Yusuke Suzuki
Comment 9 2021-02-08 01:52:58 PST
Yusuke Suzuki
Comment 10 2021-02-09 03:04:39 PST
Yusuke Suzuki
Comment 11 2021-02-09 20:26:57 PST
Yusuke Suzuki
Comment 12 2021-02-10 00:29:20 PST
Yusuke Suzuki
Comment 13 2021-02-10 00:39:08 PST
Yusuke Suzuki
Comment 14 2021-02-10 13:40:32 PST
Yusuke Suzuki
Comment 15 2021-02-10 16:46:59 PST
Yusuke Suzuki
Comment 16 2021-02-10 18:11:32 PST
Yusuke Suzuki
Comment 17 2021-02-11 11:24:59 PST
EWS Watchlist
Comment 18 2021-02-11 11:26:03 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Yusuke Suzuki
Comment 19 2021-02-11 14:55:57 PST
Yusuke Suzuki
Comment 20 2021-02-11 15:13:12 PST
Yusuke Suzuki
Comment 21 2021-02-11 20:18:34 PST
Yusuke Suzuki
Comment 22 2021-02-11 20:26:44 PST
Yusuke Suzuki
Comment 23 2021-02-11 22:52:30 PST
Yusuke Suzuki
Comment 24 2021-02-12 01:12:45 PST
Yusuke Suzuki
Comment 25 2021-02-12 22:57:40 PST
Yusuke Suzuki
Comment 26 2021-02-17 01:58:07 PST
Yusuke Suzuki
Comment 27 2021-02-18 23:02:39 PST
Yusuke Suzuki
Comment 28 2021-02-18 23:36:33 PST
Yusuke Suzuki
Comment 29 2021-02-19 11:26:31 PST
Saam Barati
Comment 30 2021-02-19 19:37:10 PST
Comment on attachment 420996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420996&action=review r=me Do we want some more tests here? Especially for various error cases, and various complex programs and such? > Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:172 > + // FIXME: All callers to createDOMException need to pass in the correct global object. > + // For now, we're going to assume the lexicalGlobalObject. Which is wrong in cases like this: > + // frames[0].document.createElement(null, null); // throws an exception which should have the subframe's prototypes. maybe file a bug for this? > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:332 > +void ScriptModuleLoader::notifyFinished(ModuleScriptLoader& moduleScriptLoader, URL&& sourceURL, Ref<DeferredPromise> promise) nit: why not just take URL here (callers can still move into the parameter)? That way we know this function takes ownership of URL. I don't think this sourceURL parameter is actually moved anywhere here, or at least there are early return paths that don't take ownership over it. > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:404 > + rejectToPropagateNetworkError(promise.get(), ModuleFetchFailureKind::WasCanceled, "Importing a module script is canceled."_s); nit: I wonder if this error should be more like "Loading a module script was cancelled" or "Load for importing a module script was canceled" > Source/WebCore/workers/WorkerGlobalScope.cpp:282 > + return Exception { TypeError, "importScripts cannot be used if woerker type is \"module\""_s }; "woerker" => "worker" > Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:364 > + // Overwrite the detailed error with a generic error. nit: not sure this comment is adding anything > Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:425 > + // Overwrite the detailed error with a generic error. ditto I wonder if we can share this code w/ above that seems to do a similar thing.
Yusuke Suzuki
Comment 31 2021-02-20 11:47:59 PST
Comment on attachment 420996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420996&action=review I think all error things are already tested. So, it is OK :) >> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:172 >> + // frames[0].document.createElement(null, null); // throws an exception which should have the subframe's prototypes. > > maybe file a bug for this? Filed https://bugs.webkit.org/show_bug.cgi?id=222229 >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:332 >> +void ScriptModuleLoader::notifyFinished(ModuleScriptLoader& moduleScriptLoader, URL&& sourceURL, Ref<DeferredPromise> promise) > > nit: why not just take URL here (callers can still move into the parameter)? That way we know this function takes ownership of URL. I don't think this sourceURL parameter is actually moved anywhere here, or at least there are early return paths that don't take ownership over it. Because we would like to remove registered / kept loader from m_loaders, we need to pass Loader. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:404 >> + rejectToPropagateNetworkError(promise.get(), ModuleFetchFailureKind::WasCanceled, "Importing a module script is canceled."_s); > > nit: I wonder if this error should be more like "Loading a module script was cancelled" or "Load for importing a module script was canceled" I'll open a bug for that since this error message is used in usual non-worker module loaders. >> Source/WebCore/workers/WorkerGlobalScope.cpp:282 >> + return Exception { TypeError, "importScripts cannot be used if woerker type is \"module\""_s }; > > "woerker" => "worker" Fixed. >> Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:364 >> + // Overwrite the detailed error with a generic error. > > nit: not sure this comment is adding anything Removed >> Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:425 >> + // Overwrite the detailed error with a generic error. > > ditto > > I wonder if we can share this code w/ above that seems to do a similar thing. Currently, not sharable since a lot of things are different (the above is loading synchronously, and this is loading asynchronously). This one is for Worklets and the above one is for Workers. And they have different semantics to load modules.
Yusuke Suzuki
Comment 32 2021-02-20 12:30:26 PST
Aakash Jain
Comment 33 2021-02-20 14:45:59 PST
(In reply to Yusuke Suzuki from comment #32) > Committed r273203 (234389@main): <https://commits.webkit.org/234389@main> This seems to have broken GTK build. 234389@main failed in https://build.webkit.org/#/builders/43/builds/332 234388@main passed in https://build.webkit.org/#/builders/43/builds/331 From https://build.webkit.org/#/builders/43/builds/332/steps/8/logs/errors: WorkerModuleScriptLoader.h:46:94: error: ‘WorkerScriptFetcher’ has not been declared
Mark Lam
Comment 34 2021-02-20 15:41:06 PST
(In reply to Aakash Jain from comment #33) > (In reply to Yusuke Suzuki from comment #32) > > Committed r273203 (234389@main): <https://commits.webkit.org/234389@main> > This seems to have broken GTK build. > 234389@main failed in https://build.webkit.org/#/builders/43/builds/332 > 234388@main passed in https://build.webkit.org/#/builders/43/builds/331 > > > From https://build.webkit.org/#/builders/43/builds/332/steps/8/logs/errors: > > WorkerModuleScriptLoader.h:46:94: error: ‘WorkerScriptFetcher’ has not been > declared Already fixed in http://trac.webkit.org/r273205, but perhaps a better fix can be implemented e.g. forward declaring WorkerScriptFetcher instead of #include'ing WorkerScriptFetcher.h.
Tobias Uhlig
Comment 35 2021-04-24 02:09:25 PDT
Created attachment 426977 [details] Advanced use case Hi guys, I just installed the tech preview to test it with a pretty complex use case. https://neomjs.github.io/pages/node_modules/neo.mjs/apps/covid/index.html Looks really good. Great job! I can finally remove the warning message and add Safari to the list of supported browsers once this hits the non tech preview version (well, except for the shared worker demos). Best regards, Tobias
Tobias Uhlig
Comment 36 2021-04-24 04:22:40 PDT
I wrote a quick blog post on this one: https://tobiasuhlig.medium.com/safari-webkit-about-to-release-support-for-js-modules-inside-the-worker-scope-9dd33fc20190?source=friends_link&sk=db61a01446ca5a51ee2f932aad9ca890 Worth a look for the dom node rotation (direction) issue. Best regards, Tobias
Yusuke Suzuki
Comment 37 2021-04-24 13:39:17 PDT
(In reply to Tobias Uhlig from comment #35) > Created attachment 426977 [details] > Advanced use case > > Hi guys, > > I just installed the tech preview to test it with a pretty complex use case. > https://neomjs.github.io/pages/node_modules/neo.mjs/apps/covid/index.html > > Looks really good. Great job! > > I can finally remove the warning message and add Safari to the list of > supported browsers once this hits the non tech preview version (well, except > for the shared worker demos). > > Best regards, > Tobias Nice! If you find an issue, please report it to bugs.webkit.org, and CC me (ysuzuki@apple.com)! Such a report is very welcomed, and it can improve quality significantly.
Thomas Steiner
Comment 38 2021-05-31 13:35:07 PDT
Is this feature coming to mobile Safari, too? It works like a charm on desktop Safari.
Jeff Schiller
Comment 39 2021-06-03 08:30:42 PDT
I haven't had time to test this, but if Safari supports this feature now/soon, can someone update https://github.com/mdn/browser-compat-data/blob/main/api/Worker.json#L173 ?
Thomas Steiner
Comment 40 2021-06-03 08:52:09 PDT
> I haven't had time to test this, but if Safari supports this feature now/soon, can someone update https://github.com/mdn/browser-compat-data/blob/main/api/Worker.json#L173 ? Just opened https://github.com/mdn/browser-compat-data/pull/10749. No support on mobile yet.
Lucas Garron
Comment 41 2021-06-23 23:48:48 PDT
May I ask if this is expected to be available to non-Safari browsers in iOS 15? Being able to rely on ESM for workers in a web app would significantly help with compatibility and performance (compared to current workarounds), and it would be really disappointing if that option involved leaving other iOS browsers behind.
Thomas Steiner
Comment 42 2021-06-23 23:51:07 PDT
> May I ask if this is expected to be available to non-Safari browsers in iOS 15? At least it is currently supported in the latest iOS 15.0 (19A5261w). I just checked in Chrome for iOS.
Note You need to log in before you can comment on or make changes to this bug.