Summary: | JS Modules in Workers | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aakash_jain, adam, annulen, anthony, ashley, benjamin, bjonesbe, briankimpossible, bugs.webkit.org, calvaris, cdumez, chi187, clopez, cyb.ai.815, daflair, dbates, dpaddock, eric.carlson, esprehn+autocc, ews-watchlist, glenn, guybedford, gyuyoung.kim, japhet, jeffschiller, jer.noble, kangil.han, kdmitrenko, keith_miller, kondapallykalyan, lgarron, mark.lam, michel.nizette, mjs, mkwst, msaboff, philipj, ryuan.choi, saam, sergio, sgospodarets, tobiasuhlig78, tomac, tzagallo, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 164425 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 147340, 165724, 192329 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2016-11-17 01:18:38 PST
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. Note that supporting dynamic `import()` in Web Workers can also be a first start along these lines. 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 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 Created attachment 402347 [details]
Patch
WIP
Created attachment 419557 [details]
Patch
Created attachment 419562 [details]
Patch
Created attachment 419565 [details]
Patch
Created attachment 419694 [details]
Patch
Created attachment 419807 [details]
Patch
Created attachment 419819 [details]
Patch
Created attachment 419821 [details]
Patch
Created attachment 419895 [details]
Patch
Created attachment 419922 [details]
Patch
Created attachment 419935 [details]
Patch
Created attachment 420011 [details]
Patch
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 Created attachment 420047 [details]
Patch
Created attachment 420050 [details]
Patch
Created attachment 420084 [details]
Patch
Created attachment 420086 [details]
Patch
Created attachment 420100 [details]
Patch
Created attachment 420102 [details]
Patch
Created attachment 420214 [details]
Patch
Created attachment 420618 [details]
Patch
Created attachment 420925 [details]
Patch
Created attachment 420927 [details]
Patch
Created attachment 420996 [details]
Patch
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 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. Committed r273203 (234389@main): <https://commits.webkit.org/234389@main> (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 (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. 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 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 (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. Is this feature coming to mobile Safari, too? It works like a charm on desktop Safari. 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 ? > 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. 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. > 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.
|