fix(lazy): parse-safe retry import, and only cache-bust native ESM

src/primitives/LazyImport.ts

@@ -8,24 +8,74 @@ import {
   sharedConfig,
 } from 'solid-js';
 
-// The chunk URL, as named in the failure message by either module system:
-// native ESM reports `Failed to fetch dynamically imported module: <url>`,
-// SystemJS reports `<url>,  (SystemJS https://…/errors.md#3)`. The first
-// http(s) URL ending in `.js` is the chunk in both cases — SystemJS's trailing
-// docs link has no `.js` in it, so it cannot be matched by mistake.
-const CHUNK_URL = /https?:\/\/\S+?\.js/;
+// Native ESM's failure message, and ONLY native ESM's. It names exactly one
+// module, and that module is always the one handed to `import()` — never one of
+// its dependencies. Chrome says `Failed to fetch dynamically imported module:
+// <url>` and Firefox `error loading dynamically imported module: <url>`; both
+// carry the phrase this matches on.
+//
+// SystemJS is deliberately NOT matched here. Its message is
+// `<failedUrl>, <parentUrl> (SystemJS …)`, and when the failure is a dependency
+// of the requested chunk the FIRST url is that dependency. Cache-busting it
+// would re-import the wrong module, and `lazy` would then read `.default` off
+// it — a blank route, or worse, someone else's component. SystemJS needs no
+// cache-buster anyway: it drops the failed load from its registry, so simply
+// calling `fn` again re-fetches.
+const NATIVE_ESM_CHUNK =
+  /dynamically imported module:?\s*(https?:\/\/\S+?\.js)/i;
 
 /**
+ * The chunk URL to retry under a cache-buster, or `undefined` when the failure
+ * is one where re-running the loader is the right move instead.
+ *
  * @internal Exported for tests. The primitives barrel re-exports `lazy` by
  * name, so this stays out of the package's public API.
  */
-export const chunkUrlFromError = (error: unknown): string | undefined =>
-  CHUNK_URL.exec(error instanceof Error ? error.message : String(error))?.[0];
+export const cacheBustableUrl = (error: unknown): string | undefined =>
+  NATIVE_ESM_CHUNK.exec(
+    error instanceof Error ? error.message : String(error),
+  )?.[1];
 
-/** @internal Exported for tests — see `chunkUrlFromError`. */
+/** @internal Exported for tests — see `cacheBustableUrl`. */
 export const cacheBust = (url: string): string =>
   `${url}${url.indexOf('?') === -1 ? '?' : '&'}chunkRetry=1`;
 
+/**
+ * A dynamic `import()`, built at runtime instead of written inline.
+ *
+ * This is not a style choice, and it must not be "simplified" back to a literal
+ * `import(url)`. The token has to stay inside a string, because this file is
+ * bundled into apps that cannot parse it:
+ *
+ * A TV app targeting an old engine may build with no SystemJS transform at all
+ * — plain `iife` output plus a syntax-only `build.target`. Rollup cannot rewrite
+ * a dynamic import in that format, and a `\/* @vite-ignore *\/` one is invisible
+ * to Vite's analysis by design, so the literal survives into the bundle. Dynamic
+ * `import()` arrived in Chrome 63; on anything older the whole script fails to
+ * *parse*, and the app never boots. That is not hypothetical: it shipped, and it
+ * took every Samsung Tizen 4.0 set (Chrome 56) offline until it was rolled back.
+ *
+ * Inside a `Function` body the token is just text until it is compiled, and that
+ * compilation is what the `try` guards. Old engines throw a SyntaxError here and
+ * a CSP without `unsafe-eval` throws an EvalError; either way we return `null`
+ * and the caller re-runs the loader instead — the same fallback every SystemJS
+ * failure already takes. Resolved once and cached, including the `null`.
+ */
+let nativeImport: ((url: string) => Promise<unknown>) | null | undefined;
+
+const getNativeImport = (): ((url: string) => Promise<unknown>) | null => {
+  if (nativeImport === undefined) {
+    try {
+      nativeImport = new Function('u', 'return import(u)') as (
+        url: string,
+      ) => Promise<unknown>;
+    } catch {
+      nativeImport = null;
+    }
+  }
+  return nativeImport;
+};
+
 // lazy load a function component asynchronously
 export function lazy<T extends Component<any>>(
   fn: () => Promise<{ default: T }>,
@@ -40,30 +90,36 @@ export function lazy<T extends Component<any>>(
   // again, not even on a later navigation. One retry turns the common transient
   // failure into a marginally slower navigation.
   //
-  // The retry re-imports under a cache-busting URL rather than re-running `fn`,
-  // because re-running only works on one of the two module systems we ship to.
-  // Under SystemJS — the `@vitejs/plugin-legacy` output older TV browsers run —
-  // the failed load is dropped from the registry, so calling `fn` again really
-  // does re-fetch. Native ESM does the opposite: the module map memoises the
-  // *failure*, so a second `import()` of the same specifier resolves straight to
-  // the cached rejection without ever touching the network, and the retry is a
-  // no-op precisely where it is needed. A distinct URL gets a fresh module-map
-  // entry and actually re-fetches.
+  // How it retries depends on which module system failed, because the two need
+  // opposite things.
+  //
+  // SystemJS — the `@vitejs/plugin-legacy` output older TV browsers run — drops
+  // the failed load from its registry, so re-running `fn` genuinely re-fetches.
+  // That is also the ONLY safe option there: its message names the failed
+  // dependency before the chunk that pulled it in, so a URL lifted out of it is
+  // frequently not the module we asked for.
   //
-  // The cost is a duplicate module record for this one chunk. Its own imports
-  // are unaffected — they resolve to their normal, already-cached URLs — so the
-  // duplication does not spread, and it only happens on a retry that would
-  // otherwise have left the route dead.
+  // Native ESM does the opposite. The module map memoises the *failure*, so a
+  // second `import()` of the same specifier resolves straight to the cached
+  // rejection without ever touching the network — the retry is a no-op exactly
+  // where it is needed. It also names one module and only one, the one handed to
+  // `import()`, so a cache-busted re-import of it is both safe and necessary: a
+  // distinct URL gets a fresh module-map entry and really re-fetches.
   //
-  // `fn` is still the fallback when the message names no URL: no worse than not
-  // retrying, and it keeps non-URL loaders (tests, custom resolvers) working.
+  // The cost there is a duplicate module record for this one chunk. Its own
+  // imports are unaffected — they resolve to their normal, already-cached URLs —
+  // so the duplication does not spread, and it only happens on a retry that
+  // would otherwise have left the route dead.
   const load = (): Promise<{ default: T }> =>
     fn().catch((error: unknown) => {
-      const url = chunkUrlFromError(error);
+      const url = cacheBustableUrl(error);
       if (url === undefined) return fn();
-      return import(/* @vite-ignore */ cacheBust(url)) as Promise<{
-        default: T;
-      }>;
+      // Null when this engine cannot compile a dynamic import, or a CSP forbids
+      // compiling one — see `getNativeImport`. Re-running the loader is then the
+      // best available retry, exactly as it is for every SystemJS failure.
+      const importer = getNativeImport();
+      if (importer === null) return fn();
+      return importer(cacheBust(url)) as Promise<{ default: T }>;
     });
 
   const wrap: T & { preload?: () => void } = ((props: any) => {

tests/lazyImport.spec.ts

@@ -1,7 +1,9 @@
+import { readFileSync } from 'node:fs';
+import { resolve } from 'node:path';
 import { describe, it, expect, vi } from 'vitest';
 import {
   cacheBust,
-  chunkUrlFromError,
+  cacheBustableUrl,
   lazy,
 } from '../src/primitives/LazyImport.ts';
 
@@ -11,33 +13,56 @@ import {
 const Page = () => null;
 const mod = { default: Page };
 
-// The two real failure messages, one per module system.
+// Real production failure messages, one per module system.
 const NATIVE_ESM_ERROR = new Error(
   'Failed to fetch dynamically imported module: https://ott.angel.com/vizio/assets/Theater.page-Bl9KRTxZ.js',
 );
+const FIREFOX_ESM_ERROR = new Error(
+  'error loading dynamically imported module: https://ott.angel.com/vizio/assets/Theater.page-Bl9KRTxZ.js',
+);
+// SystemJS error #3, direct: `<failedUrl>, <parentUrl> (SystemJS …)` with no parent.
 const SYSTEMJS_ERROR = new Error(
   'https://ott.angel.com/webos/assets/Theater.page-legacy-C2p81iuu.js,  (SystemJS https://github.com/systemjs/systemjs/blob/main/docs/errors.md#3)',
 );
+// SystemJS error #3 for a DEPENDENCY: the first URL is the dependency that
+// failed, the second is the route chunk that pulled it in. Taken verbatim from
+// production — this is the shape that made the old first-URL match wrong.
+const SYSTEMJS_DEPENDENCY_ERROR = new Error(
+  'https://ott.angel.com/webos/assets/TheaterPlayer.nav-legacy-1MzaTqJc.js, https://ott.angel.com/webos/assets/DiscoverV2Hero.page-legacy-CKnRyt-q.js (SystemJS https://github.com/systemjs/systemjs/blob/main/docs/errors.md#3)',
+);
 
-describe('chunkUrlFromError', () => {
-  it('reads the chunk URL out of a native ESM failure', () => {
-    expect(chunkUrlFromError(NATIVE_ESM_ERROR)).toBe(
+describe('cacheBustableUrl', () => {
+  it('returns the chunk named by a native ESM failure', () => {
+    expect(cacheBustableUrl(NATIVE_ESM_ERROR)).toBe(
       'https://ott.angel.com/vizio/assets/Theater.page-Bl9KRTxZ.js',
     );
   });
 
-  // The SystemJS message wraps the URL in punctuation — a trailing comma, then
-  // its own docs link. The `.js` anchor is what stops the match running past
-  // the chunk into that comma; dropping it yields a URL that 404s on retry.
-  it('reads the chunk URL, not the docs link, out of a SystemJS failure', () => {
-    expect(chunkUrlFromError(SYSTEMJS_ERROR)).toBe(
-      'https://ott.angel.com/webos/assets/Theater.page-legacy-C2p81iuu.js',
+  it("matches Firefox's wording as well as Chrome's", () => {
+    expect(cacheBustableUrl(FIREFOX_ESM_ERROR)).toBe(
+      'https://ott.angel.com/vizio/assets/Theater.page-Bl9KRTxZ.js',
     );
   });
 
+  // The regression this file exists for. SystemJS names the failed DEPENDENCY
+  // first; cache-busting it would re-import the wrong module and `lazy` would
+  // read `.default` off it. SystemJS re-fetches on a plain re-run anyway, so the
+  // right answer for every SystemJS shape is "no cache-bustable URL".
+  it('refuses the dependency URL in a SystemJS dependency failure', () => {
+    expect(cacheBustableUrl(SYSTEMJS_DEPENDENCY_ERROR)).toBeUndefined();
+  });
+
+  it('refuses a direct SystemJS failure too', () => {
+    expect(cacheBustableUrl(SYSTEMJS_ERROR)).toBeUndefined();
+  });
+
   it('returns undefined when the message names no chunk', () => {
-    expect(chunkUrlFromError(new Error('boom'))).toBeUndefined();
-    expect(chunkUrlFromError('not an error at all')).toBeUndefined();
+    expect(cacheBustableUrl(new Error('boom'))).toBeUndefined();
+    expect(cacheBustableUrl('not an error at all')).toBeUndefined();
+    // Safari phrases it without naming the module at all.
+    expect(
+      cacheBustableUrl(new Error('Importing a module script failed.')),
+    ).toBeUndefined();
   });
 });
 
@@ -61,26 +86,35 @@ describe('lazy() chunk-load retry', () => {
     expect(fn).toHaveBeenCalledTimes(1);
   });
 
-  // When the failure names a chunk, the retry must NOT re-run the loader:
-  // re-running the same specifier is exactly the no-op that leaves Vizio
-  // broken, because native ESM serves the memoised failure from the module map.
-  // The loader staying on one call is what proves the cache-busting branch ran.
+  // Native ESM: the retry must NOT re-run the loader, because re-running the
+  // same specifier is exactly the no-op that leaves it broken — the module map
+  // serves the memoised failure. The loader staying on one call is what proves
+  // the cache-busting branch ran.
   //
   // The import itself cannot resolve here — Node's ESM loader only accepts
   // `file:` and `data:` — so the promise rejects. That rejection is the test
   // environment's, not the behaviour under test, hence no assertion on its
-  // message; `chunkUrlFromError` and `cacheBust` above pin the URL that is
-  // actually requested.
+  // message; `cacheBustableUrl` and `cacheBust` above pin the URL requested.
+  it('re-imports under a cache-busting URL after a native ESM failure', async () => {
+    const fn = vi.fn().mockRejectedValue(NATIVE_ESM_ERROR);
+
+    await expect(lazy(fn).preload()).rejects.toThrow();
+    expect(fn).toHaveBeenCalledTimes(1);
+  });
+
+  // SystemJS: re-running the loader is both sufficient (the registry entry is
+  // dropped, so it really re-fetches) and necessary (its message may name a
+  // dependency rather than the requested chunk).
   it.each([
-    ['native ESM', NATIVE_ESM_ERROR],
-    ['SystemJS', SYSTEMJS_ERROR],
+    ['direct', SYSTEMJS_ERROR],
+    ['dependency', SYSTEMJS_DEPENDENCY_ERROR],
   ])(
-    're-imports under a cache-busting URL after a %s failure',
+    're-runs the loader after a SystemJS %s failure',
     async (_label, error) => {
-      const fn = vi.fn().mockRejectedValue(error);
+      const fn = vi.fn().mockRejectedValueOnce(error).mockResolvedValue(mod);
 
-      await expect(lazy(fn).preload()).rejects.toThrow();
-      expect(fn).toHaveBeenCalledTimes(1);
+      await expect(lazy(fn).preload()).resolves.toBe(mod);
+      expect(fn).toHaveBeenCalledTimes(2);
     },
   );
 
@@ -148,3 +182,58 @@ describe('lazy() preload rejection handling', () => {
     }
   });
 });
+
+describe('dynamic import is never written as a literal token', () => {
+  // The regression that took every Samsung Tizen 4.0 set (Chrome 56) offline.
+  // A `/* @vite-ignore */ import(url)` written inline survives into bundles that
+  // have no SystemJS transform — plain `iife` output with a syntax-only
+  // `build.target`. Dynamic import arrived in Chrome 63, so older engines fail
+  // to PARSE the whole script and the app never boots.
+  //
+  // A low-tech canary rather than a parser: it fails loudly if anyone
+  // "simplifies" the Function-built importer back to the inline form.
+  // Resolved from the repo root: under Vite, `import.meta.url` is not a
+  // file: URL, so readFileSync cannot take it directly.
+  const source = readFileSync(
+    resolve(process.cwd(), 'src/primitives/LazyImport.ts'),
+    'utf8',
+  );
+
+  it('does not contain the inline @vite-ignore import that shipped the outage', () => {
+    expect(source).not.toContain('import(/* @vite-ignore */');
+  });
+
+  it('builds the importer through Function so the token stays inside a string', () => {
+    expect(source).toContain("new Function('u', 'return import(u)')");
+  });
+});
+
+describe('getNativeImport fallback', () => {
+  // Chrome 56 throws a SyntaxError compiling the body; a CSP without
+  // unsafe-eval throws an EvalError. Both must degrade to re-running the loader
+  // rather than surfacing, so the retry is never worse than not cache-busting.
+  it('re-runs the loader when Function cannot compile a dynamic import', async () => {
+    const RealFunction = globalThis.Function;
+
+    try {
+      // The importer is resolved once and cached at module scope, so the module
+      // has to be re-instantiated with Function already stubbed.
+      vi.resetModules();
+      globalThis.Function = function BlockedFunction() {
+        throw new EvalError('Refused to evaluate a string as JavaScript');
+      } as unknown as FunctionConstructor;
+
+      const fresh = await import('../src/primitives/LazyImport.ts');
+      const fn = vi
+        .fn()
+        .mockRejectedValueOnce(NATIVE_ESM_ERROR)
+        .mockResolvedValue(mod);
+
+      await expect(fresh.lazy(fn).preload()).resolves.toBe(mod);
+      expect(fn).toHaveBeenCalledTimes(2);
+    } finally {
+      globalThis.Function = RealFunction;
+      vi.resetModules();
+    }
+  });
+});