diff --git a/src/primitives/LazyImport.ts b/src/primitives/LazyImport.ts index c1f73c3c..1c8f692c 100644 --- a/src/primitives/LazyImport.ts +++ b/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: `, -// SystemJS reports `, (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: +// ` and Firefox `error loading dynamically imported module: `; both +// carry the phrase this matches on. +// +// SystemJS is deliberately NOT matched here. Its message is +// `, (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) | null | undefined; + +const getNativeImport = (): ((url: string) => Promise) | null => { + if (nativeImport === undefined) { + try { + nativeImport = new Function('u', 'return import(u)') as ( + url: string, + ) => Promise; + } catch { + nativeImport = null; + } + } + return nativeImport; +}; + // lazy load a function component asynchronously export function lazy>( fn: () => Promise<{ default: T }>, @@ -40,30 +90,36 @@ export function lazy>( // 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) => { diff --git a/tests/lazyImport.spec.ts b/tests/lazyImport.spec.ts index 3749e36d..77f3b9d5 100644 --- a/tests/lazyImport.spec.ts +++ b/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: `, (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(); + } + }); +});