From 8f6c226d6023a2477daa1665fc5bb4207242f93d Mon Sep 17 00:00:00 2001 From: Chris Lorenzo Date: Fri, 11 Sep 2026 12:57:50 -0400 Subject: [PATCH 1/2] fix(lazy): stop a failed preload raising an unhandled rejection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `preload()`'s internal `.then` is a fire-and-forget side effect — it caches the resolved component — while `p` is what the caller gets back. That side-effect promise had no rejection handler of its own, so a warmed route that failed to fetch raised an unhandledRejection even when the caller dutifully caught the promise it was handed, and surfaced in the host app as an uncaught exception. Give it the same `.catch(() => {})` the hydration path already uses. The real failure still reaches the caller through `p`. Co-Authored-By: Claude Opus 5 --- src/primitives/LazyImport.ts | 10 +++++++++- tests/lazyImport.spec.ts | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 tests/lazyImport.spec.ts diff --git a/src/primitives/LazyImport.ts b/src/primitives/LazyImport.ts index c5faffc6..d2bef401 100644 --- a/src/primitives/LazyImport.ts +++ b/src/primitives/LazyImport.ts @@ -50,7 +50,15 @@ export function lazy>( : null; }) as unknown as JSX.Element; }) as T; + // The `.then` here is a fire-and-forget side effect (it caches the resolved + // component); `p` is what the caller gets back. Without the `.catch` that + // side-effect promise has no rejection handler of its own, so a failed + // preload raises an unhandledrejection even when the caller dutifully catches + // the promise it was handed — a warmed route that fails to fetch would report + // as an uncaught exception. Mirrors the `.catch(() => {})` on the hydration + // path above; the real failure still reaches the caller through `p`. wrap.preload = () => - p || ((p = fn()).then((mod) => (comp = () => mod.default)), p); + p || + ((p = fn()).then((mod) => (comp = () => mod.default)).catch(() => {}), p); return wrap as T & { preload: () => Promise<{ default: T }> }; } diff --git a/tests/lazyImport.spec.ts b/tests/lazyImport.spec.ts new file mode 100644 index 00000000..857d0f63 --- /dev/null +++ b/tests/lazyImport.spec.ts @@ -0,0 +1,36 @@ +import { describe, it, expect, vi } from 'vitest'; +import { lazy } from '../src/primitives/LazyImport.ts'; + +// `preload()` is the seam: it drives the same `load()` the render paths use, so +// the retry can be exercised without standing up a renderer. + +const Page = () => null; +const mod = { default: Page }; + +describe('lazy() preload rejection handling', () => { + // A warmed route that fails to fetch must not surface as an uncaught + // exception. `preload()`'s internal `.then` is a fire-and-forget side effect, + // so without its own catch it raises an unhandledRejection even when the + // caller catches the promise it was handed. + // + // The listener goes on `process`, not `window`: under the jsdom environment a + // Node-level promise rejection is reported there, and a `window` + // 'unhandledrejection' listener never fires — a version of this test written + // that way passes whether or not the bug is present. + it('does not raise an unhandled rejection when a preload fails', async () => { + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => unhandled.push(reason); + process.on('unhandledRejection', onUnhandled); + + try { + const fn = vi.fn().mockRejectedValue(new Error('chunk gone')); + await expect(lazy(fn).preload()).rejects.toThrow('chunk gone'); + + // Node reports a rejection as unhandled a macrotask after the fact. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(unhandled).toEqual([]); + } finally { + process.off('unhandledRejection', onUnhandled); + } + }); +}); From a8ba26a9e0c266c7a382b58d9860685aa0ea0968 Mon Sep 17 00:00:00 2001 From: Chris Lorenzo Date: Fri, 11 Sep 2026 12:58:11 -0400 Subject: [PATCH 2/2] feat(lazy): retry a failed chunk import once, cache-busted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route-chunk fetch failures were the largest bucket of uncaught JS exceptions in a shipped TV app — the rejected import escapes `lazy()` and the route never renders. Because `p` memoises the promise, one transient failure is replayed for the rest of the session: the route stays broken on every later navigation, long after the network recovered. Retry exactly once. The retry re-imports under a cache-busting URL rather than re-running `fn`, because re-running only helps one of the two module systems TV apps ship to. Under SystemJS (the `@vitejs/plugin-legacy` output older 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 touching the network — a no-op precisely where the retry is needed. A distinct URL gets a fresh module-map entry and actually re-fetches. The chunk URL is recovered from the failure message, which both module systems include. When no URL can be found — a test stub, a custom resolver — it falls back to re-running `fn`, which is no worse than not retrying. Cost is a duplicate module record for the one retried chunk; its own imports resolve to their normal, already-cached URLs, so it does not spread. Verified in a real consumer build: the modern bundle emits `import(cacheBust(url))` and the legacy bundle `module.import(...)`, so the `@vite-ignore` dynamic import survives both Rollup and the SystemJS transform, and the browser-compat guard passes on the Chrome 71 floor. `src/primitives/index.ts` now re-exports `lazy` by name instead of `export *`, so the two helpers this needs for its tests stay out of the package's public API. Co-Authored-By: Claude Opus 5 --- src/primitives/LazyImport.ts | 58 +++++++++++++++++- src/primitives/index.ts | 4 +- tests/lazyImport.spec.ts | 116 ++++++++++++++++++++++++++++++++++- 3 files changed, 173 insertions(+), 5 deletions(-) diff --git a/src/primitives/LazyImport.ts b/src/primitives/LazyImport.ts index d2bef401..c1f73c3c 100644 --- a/src/primitives/LazyImport.ts +++ b/src/primitives/LazyImport.ts @@ -8,19 +8,71 @@ 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/; + +/** + * @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]; + +/** @internal Exported for tests — see `chunkUrlFromError`. */ +export const cacheBust = (url: string): string => + `${url}${url.indexOf('?') === -1 ? '?' : '&'}chunkRetry=1`; + // lazy load a function component asynchronously export function lazy>( fn: () => Promise<{ default: T }>, ): T & { preload: () => Promise<{ default: T }> } { let comp: () => T | undefined; let p: Promise<{ default: T }> | undefined; + + // Retry the import exactly once before giving up. On a TV the chunk fetch is + // the fragile part — a brief dropout as the viewer presses OK on a rail + // rejects the import, and because `p` memoises the promise that single + // rejection is replayed for the rest of the session: the route never renders + // 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. + // + // 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. + // + // `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. + const load = (): Promise<{ default: T }> => + fn().catch((error: unknown) => { + const url = chunkUrlFromError(error); + if (url === undefined) return fn(); + return import(/* @vite-ignore */ cacheBust(url)) as Promise<{ + default: T; + }>; + }); + const wrap: T & { preload?: () => void } = ((props: any) => { const ctx = sharedConfig.context; if (ctx) { const [s, set] = createSignal(); sharedConfig.count || (sharedConfig.count = 0); sharedConfig.count++; - (p || (p = fn())) + (p || (p = load())) .then((mod) => { !sharedConfig.done && (sharedConfig.context = ctx); sharedConfig.count!--; @@ -31,7 +83,7 @@ export function lazy>( comp = s; } else if (!comp) { const [s] = createResource(() => - (p || (p = fn())).then((mod) => mod.default), + (p || (p = load())).then((mod) => mod.default), ); comp = s; } @@ -59,6 +111,6 @@ export function lazy>( // path above; the real failure still reaches the caller through `p`. wrap.preload = () => p || - ((p = fn()).then((mod) => (comp = () => mod.default)).catch(() => {}), p); + ((p = load()).then((mod) => (comp = () => mod.default)).catch(() => {}), p); return wrap as T & { preload: () => Promise<{ default: T }> }; } diff --git a/src/primitives/index.ts b/src/primitives/index.ts index 176ad26e..bb0b5333 100644 --- a/src/primitives/index.ts +++ b/src/primitives/index.ts @@ -5,7 +5,9 @@ export * from './borderBox.jsx'; export * from './useMouse.js'; export * from './portal.jsx'; export * from './Lazy.jsx'; -export * from './LazyImport.js'; +// Named rather than `export *`: LazyImport also exports internal helpers for +// its tests, and those must not become part of the public API. +export { lazy } from './LazyImport.js'; export * from './Image.jsx'; export * from './Visible.jsx'; export * from './Column.jsx'; diff --git a/tests/lazyImport.spec.ts b/tests/lazyImport.spec.ts index 857d0f63..3749e36d 100644 --- a/tests/lazyImport.spec.ts +++ b/tests/lazyImport.spec.ts @@ -1,5 +1,9 @@ import { describe, it, expect, vi } from 'vitest'; -import { lazy } from '../src/primitives/LazyImport.ts'; +import { + cacheBust, + chunkUrlFromError, + lazy, +} from '../src/primitives/LazyImport.ts'; // `preload()` is the seam: it drives the same `load()` the render paths use, so // the retry can be exercised without standing up a renderer. @@ -7,6 +11,116 @@ import { lazy } from '../src/primitives/LazyImport.ts'; const Page = () => null; const mod = { default: Page }; +// The two real 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 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)', +); + +describe('chunkUrlFromError', () => { + it('reads the chunk URL out of a native ESM failure', () => { + expect(chunkUrlFromError(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('returns undefined when the message names no chunk', () => { + expect(chunkUrlFromError(new Error('boom'))).toBeUndefined(); + expect(chunkUrlFromError('not an error at all')).toBeUndefined(); + }); +}); + +describe('cacheBust', () => { + it('starts a query string when the URL has none', () => { + expect(cacheBust('https://x/a.js')).toBe('https://x/a.js?chunkRetry=1'); + }); + + it('appends to an existing query string', () => { + expect(cacheBust('https://x/a.js?v=1')).toBe( + 'https://x/a.js?v=1&chunkRetry=1', + ); + }); +}); + +describe('lazy() chunk-load retry', () => { + it('does not retry an import that succeeds', async () => { + const fn = vi.fn().mockResolvedValue(mod); + + await expect(lazy(fn).preload()).resolves.toBe(mod); + 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. + // + // 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. + it.each([ + ['native ESM', NATIVE_ESM_ERROR], + ['SystemJS', SYSTEMJS_ERROR], + ])( + 're-imports under a cache-busting URL after a %s failure', + async (_label, error) => { + const fn = vi.fn().mockRejectedValue(error); + + await expect(lazy(fn).preload()).rejects.toThrow(); + expect(fn).toHaveBeenCalledTimes(1); + }, + ); + + // A loader that is not a URL import at all — a test stub, a custom resolver — + // has no URL to bust, so re-running it is the best available retry. + it('re-runs the loader when the failure names no chunk URL', async () => { + const fn = vi + .fn() + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValue(mod); + + await expect(lazy(fn).preload()).resolves.toBe(mod); + expect(fn).toHaveBeenCalledTimes(2); + }); + + // Bounded at one retry: a TV that has genuinely lost its connection should + // surface the failure rather than sit on a blank screen retrying. + it('gives up after exactly one retry and rejects with the second error', async () => { + const fn = vi + .fn() + .mockRejectedValueOnce(new Error('first')) + .mockRejectedValueOnce(new Error('second')); + + await expect(lazy(fn).preload()).rejects.toThrow('second'); + expect(fn).toHaveBeenCalledTimes(2); + }); + + // `p` memoises the promise, so a component mounting after a preload — or + // several routes preloading at once — must not refetch the chunk. + it('loads the chunk once across repeated preloads', async () => { + const fn = vi.fn().mockResolvedValue(mod); + const Comp = lazy(fn); + + await Promise.all([Comp.preload(), Comp.preload()]); + await Comp.preload(); + + expect(fn).toHaveBeenCalledTimes(1); + }); +}); + describe('lazy() preload rejection handling', () => { // A warmed route that fails to fetch must not surface as an uncaught // exception. `preload()`'s internal `.then` is a fire-and-forget side effect,