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

Switch purely to module workers. #214

Closed
lgarron opened this issue Sep 4, 2022 · 3 comments
Closed

Switch purely to module workers. #214

lgarron opened this issue Sep 4, 2022 · 3 comments
Labels
📦 cubing/scramble 📦 cubing/search dependencies Pull requests that update a dependency file

Comments

@lgarron
Copy link
Member

lgarron commented Sep 4, 2022

We have several workarounds for environments that don't support module workers. All environments except Firefox have already supported them for over a year: https://caniuse.com/mdn-api_worker_worker_ecmascript_modules
But Firefox is slowly making progress: https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

I don't want to break Firefox users, but I want to be ready to remove all our workarounds as soon as module worker support lands in a stable version of Firefox. Then we can tell Firefox users to update, rather than asking them to use a different browser.

Some details:

  • Our workarounds revolve around a large inline string worker for when our module worker fails to instantiate. The inline string worker is slow to download, parse, and execute. It contains a lot of code, only a subset of which is needed for any specific app. If we use the module worker exclusively, this saves significant code for any given app, and will avoid duplicated code for significant parts of the codebase (like cubing/alg).
  • We maintain a large build script to generate the source for the inline string worker, containing custom plugins:
    let latestBuildSymbol = null;
    const SEARCH_WORKER_PATH =
    "./src/cubing/search/search-worker-inside-generated-string.js";
    export const searchWorkerTarget = {
    name: "search-worker",
    builtYet: false,
    dependencies: [],
    buildSelf: async (dev) => {
    const buildSymbol = Symbol("esbuild");
    latestBuildSymbol = buildSymbol;
    const writeFile = async (buildResult) => {
    if (latestBuildSymbol !== buildSymbol) {
    console.warn("Stale `esbuild`?!");
    return;
    }
    const { contents } = buildResult.outputFiles[0];
    const contentsString = new TextDecoder("utf-8").decode(contents);
    if (typeof contentsString !== "string") {
    throw new Error(
    "Unexpected non-string for the decoded search worker source.",
    );
    }
    const workerContents = `export const workerSource = ${JSON.stringify(
    contentsString,
    )};`;
    console.log("Writing:", SEARCH_WORKER_PATH);
    writeSyncUsingTempFile(
    "search-worker-inside-generated-string.js",
    SEARCH_WORKER_PATH,
    workerContents,
    );
    };
    let watch = dev;
    if (dev) {
    watch = {
    onRebuild(error, result) {
    if (error) {
    console.error("Watch build failed:", error);
    } else {
    writeFile(result);
    }
    },
    };
    }
    const initialBuildResult = await esbuild.build({
    entryPoints: ["./src/cubing/search/inside/search-worker-js-entry.js"],
    format: "cjs",
    target: "es2020",
    bundle: true,
    watch,
    write: false,
    logLevel: "info",
    minify: !dev,
    });
    // Note that we finish writing the initial built file before we return.
    // This means that the file is in place before any dependent steps(like Snowpack) start.
    await writeFile(initialBuildResult);
    },
    };
    As the top of that file states: "Once Safari and Firefox support module workers, we can hopefully revert short commandline commands."
    • The string-based classic worker fallback is expensive, by a lot of metrics: maintenance cost, code complexity, published code size, download size, and parsing. Basic scrambling apps using cubing.js take three times the amount of downloaded code with the fallback.
    • All of our library code is 100% vanilla TypeScript, which means it can theoretically be analyzed or compiled by any tool without any special config or special steps. Except for this file, whose absence in the source tree means it needs to generated before our source becomes compilable.
    • The generated file needs to be carefully placed inside the src tree, but also excluded from git, linting, etc. Even trying to exclude it from rome will probably require more annoying, hacky workarounds.
    • The file will be re-generated by various commands, causing them to interfere each other un-necessarily. For example, make build-esm will briefly interrupt make dev, which causes an error. (Also, if EXPERIMENTAL_CUBING_JS_RELOAD_CHROME_MACOS is set, it will refresh the current page. In fact, this behaviour caused me to lose 15 minutes work while typing up this very issue.)
    • Fallback to the inline worker currently includes a timeout, which is not a great detection mechanism. I'd rather have us fail cleanly.
    • The fact that we have two instances of the worker code makes it trickier to test our workarounds for node and comlink. Those are stable right now, but I'm certain I'll have to spend a few more hours on them in the future.
    • The inline string worker fallback can mask real compatibility issues. I think we went several months where I thought we were testing the module worker, only to find that we actually weren't (because Parcel breaks our code: Parcel breaks import.meta.url in worker entry files parcel-bundler/parcel#7623).

The ecosystem is finally converging on module workers, and esbuild will soon have support for new URL("./relative.js", import.meta.url). Once esbuild has that syntax, we can switch to a single canonical implementation that works in all major bundlers (including Parcel).

Steps we need to take:

  • Now: Remove the bundle-global build, which will not be worth maintaining now that it can no longer be a single file. It's only stuck around this long because esbuild is really fast at generating it. We've been discouraging its use already (it is convenient, but it suffers from several anti-patterns and we don't make it easy to download the build), but I want to stop supporting it. There are workarounds for anyone who wants to do "flat-file" development, such as using the CDN or vendoring the ESM build into a folder. If anyone finds an important use case that we'd be leaving behind, we'll hopefully be able to learn about this before we break our ability to support it.
  • After Firefox stable has support: Remove all library and build code dedicated to search-worker-inside-generated-string.js. Document the removal of the inline string worker fallback and provide a canonical reference on compatibility.
lgarron added a commit that referenced this issue Sep 4, 2022
We've been discouraging the `bundle-global` build for a long time, and
it will get infeasible to maintain once we stop supporting the inline
string worker workaround. See
#214
lgarron added a commit that referenced this issue Sep 4, 2022
Release notes:

- [bundle-global] Remove the `bundle-global` build. See #214
@lgarron lgarron added dependencies Pull requests that update a dependency file 📦 cubing/scramble 📦 cubing/search labels Sep 9, 2022
@lgarron
Copy link
Member Author

lgarron commented Dec 16, 2022

https://bugzilla.mozilla.org/show_bug.cgi?id=1247687 has been marked as FIXED. This is fantastic news, although I haven't been able to verify the fix.

It looks like it might be possible to instantiate module workers, but they error with Error: Dynamic import not supported in this context if they encounter any dynamic imports. This is significant progress, but it still blocks one of the main benefit of module workers (to load only the code that is needed, shared with non-worker coder). The relevant issue seems to be https://bugzilla.mozilla.org/show_bug.cgi?id=1540913

@lgarron
Copy link
Member Author

lgarron commented Dec 19, 2022

https://bugzilla.mozilla.org/show_bug.cgi?id=1247687 has been marked as FIXED. This is fantastic news, although I haven't been able to verify the fix.

... and reverted, for now: https://bugzilla.mozilla.org/show_bug.cgi?id=1247687#c77

lgarron added a commit that referenced this issue May 29, 2023
Release notes:

- [search] Remove the string-based classic worker instantiation fallback. With the release of Firefox 114, all modern browsers support module workers. This allows us to remove a significant amount of code, and load some `cubing.js`-based apps with 1/3 of the code. We will not support fallbacks going forward. For more details, see: #214
@lgarron lgarron mentioned this issue Jun 4, 2023
2 tasks
@lgarron
Copy link
Member Author

lgarron commented Jun 4, 2023

This has been merged. #273 covers the release process.

@lgarron lgarron closed this as completed Jun 4, 2023
lgarron added a commit that referenced this issue Jun 6, 2023
Release notes:

- Switch purely to module workers.
  - As of [Firefox 114](https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/114), all modern JavaScript browsers and runtimes with worker support are now able to instantiate module workers. This is important for performance, as it significantly improves worker loading for `cubing.js` (requiring ⅓ as much code as before): #214
  - Due to the heavy complexity and maintenance burden of alternatives to module workers, `cubing.js` is dropping support for "classic" workers and there is no longer a "catch-all" fallback when worker instantiation fails. In removing this fallback, have carefully made sure `cubing.js` scrambles will continue work out of the box with all modern browsers, as well as the main runtimes that support web worker (`node` and `deno`). We have also tested compatibility against major bundlers. However, if you are passing `cubing.js` through your own choice of bundler, then it must correctly bundle the worker "entry file" using one of the following:
    - [`import.meta.resolve(…)`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import.meta/resolve) — this is the only "officially supported" way that we plan to support indefinitely, and the only one that will work without showing a warning in the JavaScript console.
    - One of two variants of using `new URL(…, import.meta.url)` as an alternative to `import.meta.resolve(…)`.
    - Using this workaround for `esbuild`: evanw/esbuild#312 (comment)
lgarron added a commit that referenced this issue Jun 6, 2023
Release notes:

- Switch purely to module workers.
  - As of [Firefox 114](https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/114), all modern JavaScript browsers and runtimes with worker support are now able to instantiate module workers. This is important for performance, as it significantly improves worker loading for `cubing.js` (requiring ⅓ as much code as before): #214
  - Due to the heavy complexity and maintenance burden of alternatives to module workers, `cubing.js` is dropping support for "classic" workers and there is no longer a "catch-all" fallback when worker instantiation fails. In removing this fallback, have carefully made sure `cubing.js` scrambles will continue work out of the box with all modern browsers, as well as the main runtimes that support web worker (`node` and `deno`). We have also tested compatibility against major bundlers. However, if you are passing `cubing.js` through your own choice of bundler, then it must correctly bundle the worker "entry file" using one of the following:
    - [`import.meta.resolve(…)`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import.meta/resolve) — this is the only "officially supported" way that we plan to support indefinitely, and the only one that will work without showing a warning in the JavaScript console.
    - One of two variants of using `new URL(…, import.meta.url)` as an alternative to `import.meta.resolve(…)`.
    - Using this workaround for `esbuild`: evanw/esbuild#312 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 cubing/scramble 📦 cubing/search dependencies Pull requests that update a dependency file
Projects
None yet
Development

No branches or pull requests

1 participant