Support LoadScript in module-type workers#18144
Conversation
There was a problem hiding this comment.
Thanks for the PR! The problem is real and the fix direction makes sense — module workers define importScripts but throw when you call it, so we need a fallback. A couple of suggestions to tighten this up:
Avoid new Function() — it's essentially eval() and will be blocked under strict CSP policies. There's no need for the indirection here; a plain import(scriptUrl) with a variable argument is already dynamic, and bundlers won't try to resolve it statically. If you want to be extra cautious you can add /* webpackIgnore: true */ inside the import call, but we prefer not to have bundle-specific code in core.
Drop the message sniffing — checking e.message.includes("importScripts") is fragile across browsers (Chrome, Firefox, Safari all word the error differently, and it can change between versions). The good news is instanceof TypeError alone is already a reliable discriminator: when importScripts fails due to a network error or a bad script it throws a DOMException/NetworkError, not a TypeError. A TypeError specifically means the function is unavailable or disabled, which is exactly the module worker case. So you can simplify to just:
(note - edited to add /* webpackIgnore: true */)
} catch (e) {
if (e instanceof TypeError) {
/* webpackIgnore: true */ import(scriptUrl).then(
() => onSuccess?.(),
(importError) => onError?.(`Unable to load script '${scriptUrl}' in worker`, importError)
);
} else {
onError?.(`Unable to load script '${scriptUrl}' in worker`, e);
}
}This way there's no eval, no string matching, and no eslint disables needed. One thing worth noting in the PR description is that this changes the timing from synchronous (importScripts) to asynchronous (import()) in the module worker case — but since the callers already use the callback/promise pattern, that should be fine.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Reviewer - this PR has made changes to one or more package.json files. |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18144/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18144/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18144/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
Smart Filters Editor is available to test at: |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
Chatted with @RaananW and his thought is that the potential CSP problem is worse than the problem of polluting the codebase with bundler specific directives, so let's go back to the bundler directive idea. Sorry for the confusion! |
|
Don't we still need the WebPack directive so there is not a warning by default? |
|
@wmurphyrd any update on the PR ? |
Add unit tets Add integration tests
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Reviewer - this PR has made changes to one or more package.json files. |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
PR #18144 introduced a dynamic import of a variable, which bundlers cannot "erase" even when you disable code splitting. These leaves the `import` expression as is, and dynamic imports are not supported in Babylon Native, so it causes a syntax error when loading the Babylon.js bundle. For now, we need to revert this change until we find another solution (probably either factoring the code in a way that the browser based script loading can be tree shaken for native, or dynamic imports are supported in native, or something else).
5 participants
Workers with
type: "module"cannot useimportScripts, so this falls back toimportto allow use of NullEngine (specifically, loading compressed glbs which triggers this LoadScript call) in module workersFixes https://forum.babylonjs.com/t/cannot-load-gltf-model-in-a-module-type-web-worker-due-to-importscripts-dependency/62764