Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS Cache loader errors due to trying to instantiate a script by accessing document inside a web worker #9051

Closed
lgarron opened this issue May 29, 2023 · 5 comments
Labels
Stale Inactive issues

Comments

@lgarron
Copy link

lgarron commented May 29, 2023

🐛 bug report

Given the following repro:

# bash snippet

cat << EOF > index.html
<script type="module">
  import { randomScrambleForEvent } from "cubing/scramble";
  randomScrambleForEvent("333").then(alg => alg.log())
</script>
EOF

npm install parcel@2.9.1 cubing@0.37.0-pre1
npx parcel index.html

… Parcel hits (a compiled instance of) the following line while trying to load a web worker:

let existingScripts = document.getElementsByTagName('script');

🤔 Expected Behavior

Worker instantiation works.

😯 Current Behavior

The given line is hit. It attempts to access document in a worker and errors.

💁 Possible Solution

I'm not sure. I suspect it's because Parcel's web worker instantiation support does not accommodate when the URL is calculated using a cross-origin trampoline (which is necessary for our code) and/or indirect use of the worker constructor (which is needed to publish library code that works in node as well as the browser).

Ideally, Parcel would love to see Parcel implement import.meta.resolve(…): #8924 , as:

  • That would prevent our code from hitting this issue (which is happening in a fallback that wouldn't trigger).
  • It is now a widely available language feature: all major browsers will support it as of next week.
    • node/bun/deno also do, albeit with some caveats.
  • For relative URLs, the code can safely be the same way as new URL(…, import.meta.url).href.

🔦 Context

With the release of Firefox 114 around the corner (June 6), I'm trying to update our library to rely purely on module workers.

I've tried to implement workarounds based on various worker-related Parcel issues I've filed over the years, but I can't figure out what specifically about our current code is not working.

💻 Code Sample

See above.

🌍 Your Environment

Software Version(s)
Parcel 2.9.1
@lgarron lgarron changed the title JS Cache loader errors due to trying to instantiate a script by accessing document inside a worker JS Cache loader errors due to trying to instantiate a script by accessing document inside a web worker May 29, 2023
@mischnic
Copy link
Member

mischnic commented May 29, 2023

For Parcel to work correctly, you have to do this (syntactically, no variables or function calls inbetween):

new Worker(
  new URL("./search/worker-workarounds/search-worker-entry.js", import.meta.url),
  { type: "module" }
)

If not, Parcel doesn't know if some file is running in a worker or not and cannot select the correct helper (as you've noticed)

const LOADERS = {
browser: {
css: './helpers/browser/css-loader',
html: './helpers/browser/html-loader',
js: './helpers/browser/js-loader',
wasm: './helpers/browser/wasm-loader',
IMPORT_POLYFILL: './helpers/browser/import-polyfill',
},
worker: {
js: './helpers/worker/js-loader',
wasm: './helpers/worker/wasm-loader',
IMPORT_POLYFILL: false,
},
node: {
css: './helpers/node/css-loader',
html: './helpers/node/html-loader',
js: './helpers/node/js-loader',
wasm: './helpers/node/wasm-loader',
IMPORT_POLYFILL: null,
},
};
function getLoaders(
ctx: Environment,
): ?{[string]: string, IMPORT_POLYFILL: null | false | string, ...} {
if (ctx.isWorker()) return LOADERS.worker;
if (ctx.isBrowser()) return LOADERS.browser;
if (ctx.isNode()) return LOADERS.node;
return null;
}

Webpack behaves the same, see the yellow warning here: https://webpack.js.org/guides/web-workers/


If I do this in node_modules/cubing/dist/esm/chunk-6XZN5QCJ.js, your example works.

Bildschirm­foto 2023-05-29 um 13 31 04

@mischnic
Copy link
Member

Ideally, Parcel would love to see Parcel implement import.meta.resolve(…): #8924 , as:

There's still this question: #8924 (comment)

(which is needed to publish library code that works in node as well as the browser).

One solution here might be to have a version of the code I posted above and then whatever fallback you need for Node/non-bundler browser use, but select between these versions with some if-condition/exports/package.json alias/browser field

@lgarron
Copy link
Author

lgarron commented May 29, 2023

Thanks for the replies!

For Parcel to work correctly, you have to do this (syntactically, no variables or function calls inbetween):

As mentioned above, we unfortunately cannot do this if we want our non-fallback code paths to be compatible across environments. 😕

I'll see if I can add it without too many hacks, but even after removing support for classic workers we still have to maintain a lot of code paths for cross-compatible worker support and I want to avoid maintaining an additional set of workarounds to the existing workarounds (which are already expensive to maintain, since they trigger bugs like the one documented here).

One solution here might be to have a version of the code I posted above and then whatever fallback you need for Node/non-bundler browser use, but select between these versions with some if-condition/exports/package.json alias/browser field

For a variety of reasons, we only publish a single ESM version of our library, so that is not an option for us either. 😞

There's still this question: #8924 (comment)

I still think the answer to that one is crystal-clear — there is one way to implement it that preserves the semantics of import resolution, and solves a critical problem that library authors like me cannot work around in bundlers that do not support it. And since it's new as of a year ago, different syntax from new URL(…, import.meta.url) there isn't really a backwards compat concern.

I believe I've done all the advocacy on that issue that I can, let me know if there's something more I can do to help. 🤓

lgarron added a commit to cubing/cubing.js that referenced this issue May 29, 2023
This works around parcel-bundler/parcel#9051, which makes it impossible for us to recover from instantiation errors using the more portable `new URL(…, import.meta.url)` strategy.
@lgarron
Copy link
Author

lgarron commented May 29, 2023

I'll see if I can add it without too many hacks, but even after removing support for classic workers we still have to maintain a lot of code paths for cross-compatible worker support and I want to avoid maintaining an additional set of workarounds to the existing workarounds (which are already expensive to maintain, since they trigger bugs like the one documented here).

Okay, I've implemented this workaround and I think we can live with it for now. But I'd still love to see this particular issue addressed (or #8924 implemented), so that we have fewer constraints on what workarounds we can use in what order.

lgarron added a commit to cubing/cubing.js that referenced this issue May 30, 2023
This works around parcel-bundler/parcel#9051, which makes it impossible for us to recover from instantiation errors using the more portable `new URL(…, import.meta.url)` strategy.
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs.

@github-actions github-actions bot added the Stale Inactive issues label Mar 24, 2024
@github-actions github-actions bot closed this as completed Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive issues
Projects
None yet
Development

No branches or pull requests

2 participants