JS Modules in Workers

[Yusuke Suzuki]

...

[Saleh Abdel Motaal]

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]

Note that supporting dynamic `import()` in Web Workers can also be a first start along these lines.

[Tobias Uhlig]

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]

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]

<rdar://problem/64348230>

[Yusuke Suzuki]

Created attachment 402347 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 419557 [details]
Patch

[Yusuke Suzuki]

Created attachment 419562 [details]
Patch

[Yusuke Suzuki]

Created attachment 419565 [details]
Patch

[Yusuke Suzuki]

Created attachment 419694 [details]
Patch

[Yusuke Suzuki]

Created attachment 419807 [details]
Patch

[Yusuke Suzuki]

Created attachment 419819 [details]
Patch

[Yusuke Suzuki]

Created attachment 419821 [details]
Patch

[Yusuke Suzuki]

Created attachment 419895 [details]
Patch

[Yusuke Suzuki]

Created attachment 419922 [details]
Patch

[Yusuke Suzuki]

Created attachment 419935 [details]
Patch

[Yusuke Suzuki]

Created attachment 420011 [details]
Patch

[EWS Watchlist]

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]

Created attachment 420047 [details]
Patch

[Yusuke Suzuki]

Created attachment 420050 [details]
Patch

[Yusuke Suzuki]

Created attachment 420084 [details]
Patch

[Yusuke Suzuki]

Created attachment 420086 [details]
Patch

[Yusuke Suzuki]

Created attachment 420100 [details]
Patch

[Yusuke Suzuki]

Created attachment 420102 [details]
Patch

[Yusuke Suzuki]

Created attachment 420214 [details]
Patch

[Yusuke Suzuki]

Created attachment 420618 [details]
Patch

[Yusuke Suzuki]

Created attachment 420925 [details]
Patch

[Yusuke Suzuki]

Created attachment 420927 [details]
Patch

[Yusuke Suzuki]

Created attachment 420996 [details]
Patch

[Saam Barati]

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 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]

Committed r273203 (234389@main): <https://commits.webkit.org/234389@main>

[Aakash Jain]

(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]

(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]

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]

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]

(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 ([e-mail])! Such a report is very welcomed, and it can improve quality significantly.

[Thomas Steiner]

Is this feature coming to mobile Safari, too? It works like a charm on desktop Safari.

[Jeff Schiller]

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]

> 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]

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]

> 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.