Auto optimize deps in development #2106

frandiox · 2024-05-13T11:49:26Z

Problem

Workerd cannot run CJS. Certain dependencies are only published in CJS, or are published as ESM but incorrectly call CJS APIs and break at runtime. So far, we need to tell Vite to optimize these dependencies manually by adding them to Vite's ssr.optimizeDeps.include. This is really bad DX and there's not much we can do to figure out what dependencies are like this, because we don't know them until they break the app.

Proposal

This PR catches the errors related to loading files in workerd and tries to figure out what dependencies are related to the error. Then, it adds them automatically to the user's vite.config.js and restarts the server.

Example error in the terminal and browser that we get so far:

ReferenceError: module is not defined
    at /code/node_modules/.pnpm/use-sync-external-store@1.2.0_react@18.3.1/node_modules/use-sync-external-store/shim/with-selector.js:6:3
    at Object.runViteModule (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1181:17)
    at ViteRuntime.directRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1026:78)
    at ViteRuntime.cachedRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:949:28)
    at request (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:976:128)
    at /@fs/code/node_modules/.pnpm/zustand@4.5.2_@types+react@18.3.1_immer@10.1.1_react@18.3.1/node_modules/zustand/esm/index.mjs?v=2603acb1:3:31
    at Object.runViteModule (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1181:11)
    at ViteRuntime.directRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:1026:60)
    at ViteRuntime.cachedRequest (code/node_modules/.pnpm/@shopify+mini-oxygen@3.0.2_vite@5.2.11/node_modules/@shopify/mini-oxygen/dist/vite/worker-entry.js:950:79)
    at /app/lib/app-state.tsx:3:31

This will be now caught, processed, and fixed automatically:

Tophat

Try adding CJS dependencies such as use-sync-external-store (import from use-sync-external-store or use-sync-external-store/shim).
You can also disable any already optimized dep in the Hydrogen plugin (except react/jsx-dev-runtime which is a bit special and doesn't work) to see how they are added to the user config automatically.

This should work ideally without opening the browser. However, if you do open the browser or change imports during development, it should still ideally work.

Note

~~I still need to test with PNPM. It might not work there yet because paths in node_modules are a bit different so it might need some extra tinkering.~~ I've tested with demo-store + PNPM and it seems to work (at least after this change, but it's probably a rare thing with a very broken dependency).

Errors

Browser error page

Using minimal CSS because this page is not rendered by Remix (plain strings):

When no `ssr.optimizeDeps.include` array is found in vite.config.ts

The following error shows up. We could try adding it automatically but it might be error prone.

When running with `--disable-deps-optimizer` flag

We don't write automatically anymore but we still read the error, parse the stack and try to provide helpful information:

Other non-CJS reference errors

Not every ReferenceError comes from an optimizable dependency. If the user has bad code it will be shown like this:

When we can't find the issue

In this situation we tried to optimize textr but that did not solve the issue. In this case, we print the stack for manual fixing. The user should find typographic-base/index in this case:

frandiox · 2024-05-13T11:51:58Z

packages/cli/src/lib/deps-optimizer.ts

+            clearTimeout(showBannerUrlTimeout);
+            showBannerUrlTimeout = setTimeout(showSuccessBanner, 2000);


This is a best-effort debouncing to show the success banner after all of the dependencies are added to the list.

frandiox · 2024-05-13T11:52:54Z

packages/cli/src/lib/deps-optimizer.ts

+      const unknownError = new BugError(headline + ': ' + colors.dim(message));
+      unknownError.stack = cleanStack;
+      renderFatalError(unknownError);


Other non-CJS reference errors like asdf is not defined should come here and be printed normally.

frandiox · 2024-05-13T11:54:39Z

packages/mini-oxygen/src/vite/deps-optimizer.ts

+    );
+  }
+
+  const result = await entryPointErrorHandler?.({optimizableDependency, stack});


This entryPointErrorHandler is passed from the CLI to the mini-oxygen plugin, and it's what writes to vite.config.js (it has access to prettier and ast-grep).

frandiox · 2024-05-13T11:56:02Z

packages/mini-oxygen/src/vite/deps-optimizer.ts

+  const filepath = stack
+    .match(/^\s+at\s([^:\?]+)(\?|:\d)/m)?.[1]
+    ?.replace(/^.*?\(/, '')
+    .replace(/\?.+$/, '');
+
+  const nodeModulesPath = filepath?.split(/node_modules[\\\/]/).pop();
+
+  if (!filepath || !nodeModulesPath) return;


Finds the filepath in the stack that comes from node_modules because we only want to handle Reference Errors in dependencies, not in-app.

frandiox · 2024-05-13T12:01:16Z

packages/mini-oxygen/src/vite/deps-optimizer.ts

+  const mods = viteServer.moduleGraph.getModulesByFile(filepath);
+  const modImporters = new Set<string>();
+
+  mods?.forEach((mod) => {
+    mod.importers.forEach((importer) => {
+      if (importer.file) modImporters.add(importer.file);
+    });
+  });
+
+  for (const mod of modImporters) {
+    const importersSet = new Set<string>();
+
+    const code = await readFile(mod, 'utf-8').catch(() => '');
+    const matches =
+      code.matchAll(/import\s[^'"]+\sfrom\s+['"]((@|\w)[^'"]+)['"]/g) ?? [];
+
+    for (const [, match] of matches) {
+      importersSet.add(match);
+    }
+
+    const importers = Array.from(importersSet).sort(
+      (a, b) => b.length - a.length,
+    );
+
+    const foundMatchingDependency = importers.find((importer) =>
+      nodeModulesPath.startsWith(importer),
+    );
+
+    if (foundMatchingDependency) return foundMatchingDependency;
+  }


Vite knows which modules imported the file that has the CJS syntax. We get all of them and read their source code. Then, we try to find the exact string that is used to import the offending file.

Examples:

With the stack line .../node_modules/react-dom/server.browser.js we'll find a file containing import xyz from 'react-dom/server', and we get the string react-dom/server to add it to optimizeDeps.

With the stack line .../node_modules/set-cookie-parser/lib/index.js we'll find a file containing import xyz from 'set-cookie-parser', and we get the string set-cookie-parser to add it to optimizeDeps.

frandiox · 2024-05-13T12:01:52Z

packages/mini-oxygen/src/vite/plugin.ts

-          ? entry
-          : path.resolve(resolvedConfig.root, entry);
+      configureServer: {
+        order: 'pre',


Adding pre here means we always run before Remix plugin, thus fixing the situation where the user adds remix(), oxygen() in the wrong order.

frandiox · 2024-05-13T12:05:02Z

packages/mini-oxygen/src/vite/plugin.ts

-        const miniOxygenPromise = Promise.resolve(apiOptions.envPromise).then(
-          (remoteEnv) =>
-            startMiniOxygenRuntime({
-              entry,
-              viteDevServer,
-              crossBoundarySetup: apiOptions.crossBoundarySetup,
-              env: {...remoteEnv, ...apiOptions.env, ...pluginOptions.env},
-              debug: apiOptions.debug ?? pluginOptions.debug ?? false,
-              inspectorPort:
-                apiOptions.inspectorPort ?? pluginOptions.inspectorPort,
-              requestHook: apiOptions.requestHook,
-              logRequestLine:
-                // Give priority to the plugin option over the CLI option here,
-                // since the CLI one is just a default, not a user-provided flag.
-                pluginOptions?.logRequestLine ?? apiOptions.logRequestLine,
-            }),
-        );


We were starting the MiniOxygen instance very early before. This has an issue where the instance could be disposed when the Vite server is disposed, but not restarted because this code doesn't run again.

I've delayed starting the instance until we get the first request. If the instance has been disposed, it will start it again the first time it's needed.

This happens every time you save vite.config.js.

frandiox · 2024-05-13T12:05:57Z

packages/mini-oxygen/src/vite/server-middleware.ts

+    const ready =
+      miniOxygen && !miniOxygen.isDisposed
+        ? Promise.resolve()
+        : getMiniOxygenOptions().then((options) => {
+            miniOxygenOptions = options;
+            miniOxygen = startMiniOxygenRuntime(options);
+          });


Create or restart the instance if it has been disposed (when running viteServer.close(), which happens on vite.config.js save automatically).

frandiox · 2024-05-13T12:06:47Z

packages/mini-oxygen/src/vite/server-middleware.ts

+  const warmupWorkerdCache = async () => {
+    await new Promise((resolve) => setTimeout(resolve, 200));
+
+    const viteUrl = getViteUrl(viteDevServer);
+
+    if (viteUrl) {
+      fetch(new URL(WARMUP_PATHNAME, viteUrl)).catch(() => {});
+    }
+  };
+
+  viteDevServer.httpServer?.listening
+    ? warmupWorkerdCache()
+    : viteDevServer.httpServer?.once('listening', warmupWorkerdCache);


This warmup request happens before the browser is open. Therefore, we still create a MiniOxygen instance quite early and start discovering unoptimized deps.

frandiox · 2024-05-13T12:07:19Z

packages/mini-oxygen/src/vite/worker-entry.ts

+          status: 503,
+          statusText: 'executeEntrypoint error',


This just signals we couldn't load the entry point and MiniOxygen will log information accordingly in the Node/Vite process.

…cluded

shopify · 2024-05-14T04:42:27Z

Oxygen deployed a preview of your fd-auto-optimize-deps branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
vite	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 20, 2024 1:35 AM
custom-cart-method	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 20, 2024 1:35 AM
third-party-queries-caching	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 20, 2024 1:35 AM
Skeleton (skeleton.hydrogen.shop)	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 20, 2024 1:35 AM
optimistic-cart-ui	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 20, 2024 1:35 AM
subscriptions	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 20, 2024 1:35 AM

Learn more about Hydrogen's GitHub integration.

wizardlyhel

works like a charm

juanpprieto

💯 💯 💯 💯

gfscott

Very cool! 💯 Just some small tweaks to error message wording etc.

templates/skeleton/vite.config.ts

gfscott · 2024-05-15T15:11:31Z

packages/cli/src/lib/deps-optimizer.ts

+                `\nAdded '${colors.yellow(
+                  optimizableDependency,
+                )}' to your Vite config's ssr.optimizeDeps.include\n`,


Vite uses present tense when restarting so I think it makes sense for us to follow suit in this context.

Suggested change

`\nAdded '${colors.yellow(

optimizableDependency,

)}' to your Vite config's ssr.optimizeDeps.include\n`,

`\nAdding '${colors.yellow(

optimizableDependency,

)}' to Vite config in ssr.optimizeDeps.include...\n`,

This log line is shown after we add it, though. If we try to log it before or during, it will be hidden by Vite's "clear screen after restart logic".
Also, it's always printed after Vite's [vite] server restarted log. That's why I wrote Added in the first place.

With this info, can you confirm if we still want Adding?

Ah, I didn't quite get what was going on there. Yeah, let's keep Added in that case!

packages/cli/src/lib/deps-optimizer.ts

packages/mini-oxygen/src/vite/entry-error.ts

blittle · 2024-05-15T20:45:18Z

packages/mini-oxygen/src/vite/entry-error.ts

+    .filter((line) => !line.includes('mini-oxygen'))
+    .join('\n');
+
+  const optimizableDependency = await findOptimizableDependency(


Seems like the logic inside findOptimizableDependency is possible to throw in some situations. If it does, it will end up in the outer catch that logs'MiniOxygen: Error during evaluation:. That has the potential to confuse the developer. I'd rather see the original error from vite, than an error in our logic trying to optimize the dependencies. Maybe we should try catch around this, and throw the original entryPointErrorHandler if it fails?

In what situation can it throw? It already has a .catch() in the only async call (readFile) and the rest shouldn't create any issue 🤔

I can catch it here as well I guess to be extra safe.

Whenever I see I/O I assume it can fail (maybe I should also test this in Windows). Also not sure if there are situations vite would fail.

blittle · 2024-05-15T20:49:53Z

I'm curious, why are "react" and "react-dom" inside there? Are those necessary? They aren't in the skeleton template vite config.

blittle · 2024-05-15T20:56:08Z

I did some testing, and it worked well! Great job! One package for some reason it did not automatically optimize is warning: https://www.npmjs.com/package/warning

Co-authored-by: Graham F. Scott <gfscott@users.noreply.github.com>

frandiox · 2024-05-16T05:48:05Z

Very cool! 💯 Just some small tweaks to error message wording etc.

@gfscott Great suggestion, thanks! Just left you a message in one of them.

I'm curious, why are "react" and "react-dom" inside there? Are those necessary? They aren't in the skeleton template vite config.

@blittle React and react-dom are CJS dependencies by design, and they probably won't be in ESM to avoid issues with importing React twice (CJS & ESM) by mistake. Therefore, these deps need to be optimized by Vite to run on workerd.
Since these deps are well known for us because we build on top of React and Remix, we are telling Vite to optimize them in our Hydrogen plugin directly so that the user doesn't need to worry about them.

What the user need to add is custom dependencies for their own app code that we don't know about. Does answer your question?

I did some testing, and it worked well! Great job! One package for some reason it did not automatically optimize is warning: https://www.npmjs.com/package/warning

@blittle It actually adds it to my file automatically:

Maybe you are thinking about the message at the end of the screenshot from Vite? That's for a different field in vite.config.js. <root>.ssr.optimizeDeps.include is what we need to fill to make it work in workerd. The message from Vite is related to <root>.optimizeDeps.include but it can figure it out by itself, so we don't need to change this field.

blittle · 2024-05-16T18:39:05Z

It looks like warning broke when I just imported it bare. Not sure why this would make a difference

frandiox · 2024-05-17T03:25:02Z

It looks like warning broke when I just imported it bare. Not sure why this would make a difference

Oh this is probably because the regular expression I'm using checks from in the import statement. I can remove it but I don't think importing for side-effects is very common in worker environments 🤔

Edit: fixed in 134d01a

frandiox added 12 commits May 13, 2024 15:54

Update package-lock after release

6be7b38

Delay starting MiniOxygen until first request to fix Vite config reloads

231adca

Deprecate plugin internal option

a8da90c

Ensure plugins work independently of order with Remix

98a0569

Extract showSuccessBanner function

a7b15f0

Add empty ssr.optimizeDeps to skeleton's Vite config

5e248e9

Add deps optimizer

65a11b7

Extract manual error page

b9ec484

Handle entry point error in MiniOxygen and show info

ade5b16

Add entryPointErrorHandler hook

c671c31

Refactor deps optimizer to use entryPointErrorHandler in CLI

aa0b269

Changesets

7ff8c83

frandiox requested a review from a team May 13, 2024 11:49

frandiox commented May 13, 2024

View reviewed changes

frandiox mentioned this pull request May 13, 2024

[VITE] Missing dependencies after running setup vite command #1834

Closed

frandiox added 3 commits May 14, 2024 12:23

Show stack error if what we want to add to optimizeDeps is already in…

ed45468

…cluded

Debounce logic to avoid too many errors

5caeadf

Avoid logging CJS errors in HMR

c01a3e6

frandiox added 2 commits May 14, 2024 14:06

Improve error styles

af485fd

Rename file

e062f21

frandiox added 2 commits May 14, 2024 14:10

Fix typecheck

ca6d751

Remove log

1132100

wizardlyhel approved these changes May 14, 2024

View reviewed changes

juanpprieto approved these changes May 14, 2024

View reviewed changes

frandiox requested a review from gfscott May 15, 2024 03:46

gfscott approved these changes May 15, 2024

View reviewed changes

blittle reviewed May 15, 2024

View reviewed changes

frandiox and others added 5 commits May 16, 2024 14:24

Apply suggestions from code review

e2b87e0

Co-authored-by: Graham F. Scott <gfscott@users.noreply.github.com>

Use header variable

c0673d3

Extra safe with promises

2ef713e

Prettier

f28ef0a

Wrong string after review commit

edd0046

frandiox added 3 commits May 17, 2024 12:32

Match imports without from

134d01a

Wording

e16dbf7

Merge branch 'main' into fd-auto-optimize-deps

7ed02f1

frandiox merged commit 27e51ab into main May 20, 2024
13 checks passed

frandiox deleted the fd-auto-optimize-deps branch May 20, 2024 02:57

shopify-github-actions-access bot mentioned this pull request May 20, 2024

[ci] release 2024-04 #2104

Merged

frandiox mentioned this pull request May 20, 2024

CommonJS dependency incompatibility: ReferenceError: require is not defined #2129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto optimize deps in development #2106

Auto optimize deps in development #2106

frandiox commented May 13, 2024 •

edited

Loading

frandiox May 13, 2024 •

edited

Loading

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

frandiox May 13, 2024

shopify bot commented May 14, 2024 •

edited

Loading

wizardlyhel left a comment

juanpprieto left a comment •

edited

Loading

gfscott left a comment

gfscott May 15, 2024

frandiox May 16, 2024

gfscott May 17, 2024

blittle May 15, 2024

frandiox May 16, 2024

blittle May 16, 2024

blittle commented May 15, 2024

blittle commented May 15, 2024

frandiox commented May 16, 2024

blittle commented May 16, 2024

frandiox commented May 17, 2024 •

edited

Loading

		clearTimeout(showBannerUrlTimeout);
		showBannerUrlTimeout = setTimeout(showSuccessBanner, 2000);

Auto optimize deps in development #2106

Auto optimize deps in development #2106

Conversation

frandiox commented May 13, 2024 • edited Loading

Problem

Proposal

Tophat

Note

Errors

Browser error page

When no ssr.optimizeDeps.include array is found in vite.config.ts

When running with --disable-deps-optimizer flag

Other non-CJS reference errors

When we can't find the issue

frandiox May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shopify bot commented May 14, 2024 • edited Loading

wizardlyhel left a comment

Choose a reason for hiding this comment

juanpprieto left a comment • edited Loading

Choose a reason for hiding this comment

gfscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blittle commented May 15, 2024

blittle commented May 15, 2024

frandiox commented May 16, 2024

blittle commented May 16, 2024

frandiox commented May 17, 2024 • edited Loading

frandiox commented May 13, 2024 •

edited

Loading

When no `ssr.optimizeDeps.include` array is found in vite.config.ts

When running with `--disable-deps-optimizer` flag

frandiox May 13, 2024 •

edited

Loading

shopify bot commented May 14, 2024 •

edited

Loading

juanpprieto left a comment •

edited

Loading

frandiox commented May 17, 2024 •

edited

Loading