Bug 164860 - JS Modules in Workers
Summary: JS Modules in Workers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 164425
Blocks: 147340 192329 165724
  Show dependency treegraph
 
Reported: 2016-11-17 01:18 PST by Yusuke Suzuki
Modified: 2021-06-23 23:51 PDT (History)
46 users (show)

See Also:


Attachments
Patch (27.74 KB, patch)
2020-06-19 16:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (28.34 KB, patch)
2021-02-07 23:33 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (28.32 KB, patch)
2021-02-08 01:26 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (29.04 KB, patch)
2021-02-08 01:52 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (66.94 KB, patch)
2021-02-09 03:04 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (97.78 KB, patch)
2021-02-09 20:26 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (102.99 KB, patch)
2021-02-10 00:29 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (111.39 KB, patch)
2021-02-10 00:39 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (111.46 KB, patch)
2021-02-10 13:40 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (140.58 KB, patch)
2021-02-10 16:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (145.20 KB, patch)
2021-02-10 18:11 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (171.03 KB, patch)
2021-02-11 11:24 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (224.68 KB, patch)
2021-02-11 14:55 PST, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (224.52 KB, patch)
2021-02-11 15:13 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (230.10 KB, patch)
2021-02-11 20:18 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (230.11 KB, patch)
2021-02-11 20:26 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (230.21 KB, patch)
2021-02-11 22:52 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (232.99 KB, patch)
2021-02-12 01:12 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (232.99 KB, patch)
2021-02-12 22:57 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (233.37 KB, patch)
2021-02-17 01:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (241.54 KB, patch)
2021-02-18 23:02 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (244.69 KB, patch)
2021-02-18 23:36 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (245.29 KB, patch)
2021-02-19 11:26 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff
Advanced use case (798.71 KB, image/png)
2021-04-24 02:09 PDT, Tobias Uhlig
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-11-17 01:18:38 PST
...
Comment 1 Saleh Abdel Motaal 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.
Comment 2 Guy Bedford 2019-09-11 11:47:33 PDT
Note that supporting dynamic `import()` in Web Workers can also be a first start along these lines.
Comment 3 Tobias Uhlig 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
Comment 4 Tobias Uhlig 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
Comment 5 Radar WebKit Bug Importer 2020-06-14 19:44:14 PDT
<rdar://problem/64348230>
Comment 6 Yusuke Suzuki 2020-06-19 16:47:17 PDT
Created attachment 402347 [details]
Patch

WIP
Comment 7 Yusuke Suzuki 2021-02-07 23:33:13 PST
Created attachment 419557 [details]
Patch
Comment 8 Yusuke Suzuki 2021-02-08 01:26:54 PST
Created attachment 419562 [details]
Patch
Comment 9 Yusuke Suzuki 2021-02-08 01:52:58 PST
Created attachment 419565 [details]
Patch
Comment 10 Yusuke Suzuki 2021-02-09 03:04:39 PST
Created attachment 419694 [details]
Patch
Comment 11 Yusuke Suzuki 2021-02-09 20:26:57 PST
Created attachment 419807 [details]
Patch
Comment 12 Yusuke Suzuki 2021-02-10 00:29:20 PST
Created attachment 419819 [details]
Patch
Comment 13 Yusuke Suzuki 2021-02-10 00:39:08 PST
Created attachment 419821 [details]
Patch
Comment 14 Yusuke Suzuki 2021-02-10 13:40:32 PST
Created attachment 419895 [details]
Patch
Comment 15 Yusuke Suzuki 2021-02-10 16:46:59 PST
Created attachment 419922 [details]
Patch
Comment 16 Yusuke Suzuki 2021-02-10 18:11:32 PST
Created attachment 419935 [details]
Patch
Comment 17 Yusuke Suzuki 2021-02-11 11:24:59 PST
Created attachment 420011 [details]
Patch
Comment 18 EWS Watchlist 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
Comment 19 Yusuke Suzuki 2021-02-11 14:55:57 PST
Created attachment 420047 [details]
Patch
Comment 20 Yusuke Suzuki 2021-02-11 15:13:12 PST
Created attachment 420050 [details]
Patch
Comment 21 Yusuke Suzuki 2021-02-11 20:18:34 PST
Created attachment 420084 [details]
Patch
Comment 22 Yusuke Suzuki 2021-02-11 20:26:44 PST
Created attachment 420086 [details]
Patch
Comment 23 Yusuke Suzuki 2021-02-11 22:52:30 PST
Created attachment 420100 [details]
Patch
Comment 24 Yusuke Suzuki 2021-02-12 01:12:45 PST
Created attachment 420102 [details]
Patch
Comment 25 Yusuke Suzuki 2021-02-12 22:57:40 PST
Created attachment 420214 [details]
Patch
Comment 26 Yusuke Suzuki 2021-02-17 01:58:07 PST
Created attachment 420618 [details]
Patch
Comment 27 Yusuke Suzuki 2021-02-18 23:02:39 PST
Created attachment 420925 [details]
Patch
Comment 28 Yusuke Suzuki 2021-02-18 23:36:33 PST
Created attachment 420927 [details]
Patch
Comment 29 Yusuke Suzuki 2021-02-19 11:26:31 PST
Created attachment 420996 [details]
Patch
Comment 30 Saam Barati 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.
Comment 31 Yusuke Suzuki 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.
Comment 32 Yusuke Suzuki 2021-02-20 12:30:26 PST
Committed r273203 (234389@main): <https://commits.webkit.org/234389@main>
Comment 33 Aakash Jain 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
Comment 34 Mark Lam 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.
Comment 35 Tobias Uhlig 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
Comment 36 Tobias Uhlig 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
Comment 37 Yusuke Suzuki 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.
Comment 38 Thomas Steiner 2021-05-31 13:35:07 PDT
Is this feature coming to mobile Safari, too? It works like a charm on desktop Safari.
Comment 39 Jeff Schiller 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 ?
Comment 40 Thomas Steiner 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.
Comment 41 Lucas Garron 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.
Comment 42 Thomas Steiner 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.