From 67777923cc6d86f08701a35868ad0eb2ee5091e2 Mon Sep 17 00:00:00 2001 From: hexplus Date: Sun, 6 Sep 2026 23:06:11 -0600 Subject: [PATCH 1/4] fix: reject unwrapped island loaders and detect thenables by shape in Suspense --- .tmp-probe/mod.ts | 1 + .tmp-probe/probe.ts | 12 ++ src/platform/islands.ts | 61 +++++++- src/plugins/router.ts | 18 ++- tests/thenable-and-loader-shapes.test.ts | 191 +++++++++++++++++++++++ 5 files changed, 275 insertions(+), 8 deletions(-) create mode 100644 .tmp-probe/mod.ts create mode 100644 .tmp-probe/probe.ts create mode 100644 tests/thenable-and-loader-shapes.test.ts diff --git a/.tmp-probe/mod.ts b/.tmp-probe/mod.ts new file mode 100644 index 0000000..b2d1314 --- /dev/null +++ b/.tmp-probe/mod.ts @@ -0,0 +1 @@ +export default (() => {}) as unknown as import("../src/platform/enhance").EnhanceSetup; diff --git a/.tmp-probe/probe.ts b/.tmp-probe/probe.ts new file mode 100644 index 0000000..bfa098b --- /dev/null +++ b/.tmp-probe/probe.ts @@ -0,0 +1,12 @@ +import { registerIsland, lazyIsland } from "../src/platform/islands"; +import type { EnhanceSetup } from "../src/platform/enhance"; + +// (1) The correct form. +registerIsland("ok", lazyIsland(() => import("./mod.js") as Promise<{ default: EnhanceSetup }>)); + +// (2) The mistake: a loader that was never wrapped. +registerIsland("oops", () => import("./mod.js") as Promise<{ default: EnhanceSetup }>); + +// (3) Is a zero-arg promise-returning fn assignable to EnhanceSetup directly? +const asSetup: EnhanceSetup = () => Promise.resolve(1) as unknown as Promise; +void asSetup; diff --git a/src/platform/islands.ts b/src/platform/islands.ts index 8a0a716..62d0a03 100644 --- a/src/platform/islands.ts +++ b/src/platform/islands.ts @@ -20,13 +20,30 @@ export type IslandStrategy = "load" | "idle" | "visible" | "interaction" | "medi * is one). Only fetched when the island activates. Wrap with {@link lazyIsland}. */ export type IslandLoader = () => Promise; +/** + * A loader that has been through {@link lazyIsland}. + * + * The brand is what makes the distinction real. An inline setup and a loader + * are both plain functions, so nothing at runtime can tell them apart before + * one is called — and `registerIsland` used to accept a bare `IslandLoader`, + * which meant a forgotten `lazyIsland(...)` type-checked and was then invoked + * as a setup: it ignored its `ctx`, returned a promise nobody awaited, and the + * island was marked enhanced although its real setup never ran. + * + * Requiring the brand moves that mistake to compile time. `EnhanceSetup` + * returns `void | (() => void)`, so a promise-returning function is not + * assignable to it — with the unbranded loader arm gone, an unwrapped loader + * no longer satisfies `IslandRegistration` at all. + */ +export type LazyIslandLoader = IslandLoader & { readonly [LAZY]: true }; + /** Either an inline setup, or a {@link lazyIsland}-branded loader. */ -export type IslandRegistration = EnhanceSetup | IslandLoader; +export type IslandRegistration = EnhanceSetup | LazyIslandLoader; /** Island ids appear in attribute selectors and registry lookups. */ const SAFE_NAME = /^[A-Za-z0-9_-]+$/; /** Brand distinguishing a lazy loader from an inline setup (both are functions). */ -const LAZY = Symbol.for("sibujs.islands.lazy"); +const LAZY: unique symbol = Symbol.for("sibujs.islands.lazy") as never; // Shared across duplicate runtime copies so islands registered through one copy // are mountable by mountIslands() called through another. @@ -41,9 +58,9 @@ const registry = globalSingleton(Symbol.for("sibujs.islands.registry.v1"), () => * registerIsland("chart", lazyIsland(() => import("./islands/chart.js"))); * ``` */ -export function lazyIsland(loader: IslandLoader): IslandLoader { +export function lazyIsland(loader: IslandLoader): LazyIslandLoader { (loader as unknown as Record)[LAZY] = true; - return loader; + return loader as LazyIslandLoader; } /** @@ -81,6 +98,40 @@ async function resolveSetup(reg: IslandRegistration): Promise { + const returned = setup(ctx) as unknown; + if (returned && typeof (returned as PromiseLike).then === "function") { + throw new Error( + `[SibuJS islands] the setup for "${name}" returned a promise. ` + + "If it is a lazy import, register it as lazyIsland(() => import(…)) — an unwrapped loader is called as a " + + "setup, so its module is never loaded. If it is an async setup, make it synchronous: enhance() is a " + + "synchronous transaction, and bindings registered after an await escape both its rollback and its disposer.", + ); + } + return returned as ReturnType; + }; +} + export interface MountIslandsOptions { /** IntersectionObserver options for the `visible` strategy. */ rootMargin?: string; @@ -163,7 +214,7 @@ export function mountIslands( // already rolled back the island's bindings/listeners — the isolation // is real lifecycle isolation, not just control flow. try { - const disposeIsland = enhance(el, setup); + const disposeIsland = enhance(el, rejectThenableSetup(name, setup)); // Teardown can also land *during* setup (setup reaches the cleanup, // directly or via a parent). The disposers list was drained before // this disposer existed, so pushing it now would strand the island diff --git a/src/plugins/router.ts b/src/plugins/router.ts index 86ac9f9..30df073 100644 --- a/src/plugins/router.ts +++ b/src/plugins/router.ts @@ -2852,11 +2852,23 @@ export function Suspense(props: { try { const result = props.nodes(); let element: HTMLElement; - if (result instanceof Promise) { + // Thenable by SHAPE, not `instanceof Promise`. + // + // `instanceof` asks which realm built the object. A promise from an + // iframe, a `vm` context, a worker bridge or a polyfill is a perfectly + // good promise and fails that test — it was then treated as a DOM node, + // `insertBefore` threw, and the boundary rendered its error branch for + // work that was about to succeed. Worse, the element the promise went on + // to resolve to was never inserted and never disposed: live reactive + // bindings attached to nothing. + // + // `await` already accepts any thenable, so shape is both the safer test + // and the one that matches what the next line actually does. + if (result != null && typeof (result as PromiseLike).then === "function") { showFallback(myGeneration); - element = await result; + element = await (result as PromiseLike); } else { - element = result; + element = result as HTMLElement; } // Re-checked *after* the await, immediately before the synchronous diff --git a/tests/thenable-and-loader-shapes.test.ts b/tests/thenable-and-loader-shapes.test.ts new file mode 100644 index 0000000..eea578c --- /dev/null +++ b/tests/thenable-and-loader-shapes.test.ts @@ -0,0 +1,191 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { div } from "../src/core/rendering/html"; +import { lazyIsland, mountIslands, registerIsland, unregisterIsland } from "../src/platform/islands"; +import { Suspense } from "../src/plugins/router"; + +// --------------------------------------------------------------------------- +// Two shapes the runtime recognised too narrowly. +// +// 1. An island loader that was never wrapped in `lazyIsland()` is a plain +// function, indistinguishable at runtime from a setup — so it was CALLED as +// one. The loader ignores its `ctx` argument, returns a promise nobody +// awaits, and `enhance()` reports success: the island is stamped +// `data-sibu-enhanced="true"` while its real setup never ran. The public +// type accepted the mistake, because `IslandRegistration` admitted any +// `IslandLoader`, branded or not. +// +// 2. `Suspense` decided "is this async?" with `instanceof Promise`, which is +// false for a promise from another realm (an iframe, a `vm` context, a +// polyfill) and for any ordinary thenable. Such a value was treated as a +// DOM node, `insertBefore` threw, the boundary rendered its error branch, +// and the element the promise later resolved to was left live and detached +// — reactive bindings still running, attached to nothing. +// --------------------------------------------------------------------------- + +const flush = async () => { + for (let i = 0; i < 20; i++) await Promise.resolve(); + await new Promise((r) => setTimeout(r, 0)); +}; + +let warn: ReturnType; +let error: ReturnType; + +beforeEach(() => { + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + error = vi.spyOn(console, "error").mockImplementation(() => {}); + document.body.innerHTML = ""; +}); +afterEach(() => { + warn.mockRestore(); + error.mockRestore(); + document.body.innerHTML = ""; +}); + +/** `div()` returns `Element`; Suspense's props are typed `HTMLElement`. */ +const el = (...args: Parameters): HTMLElement => div(...args) as HTMLElement; + +const logged = () => [...warn.mock.calls, ...error.mock.calls].map((c) => c.map((a) => String(a)).join(" ")).join("\n"); + +/** + * Compile-time half of the island fix, checked by `npm run typecheck:tests`. + * + * Never executed — its value is that `tsc` fails if the `@ts-expect-error` + * stops being an error, i.e. if `IslandRegistration` ever readmits an unbranded + * loader. The runtime tests below cover JS callers and casts that bypass this. + */ +function _islandRegistrationTypes(): void { + const loader = () => Promise.resolve({ default: () => {} }); + + // @ts-expect-error — an unwrapped loader is not a valid registration + registerIsland("compile-unwrapped", loader); + + registerIsland("compile-wrapped", lazyIsland(loader)); + registerIsland("compile-inline", () => {}); +} +void _islandRegistrationTypes; + +describe("an island loader that was never wrapped in lazyIsland()", () => { + afterEach(() => unregisterIsland("chart")); + + it("is not reported as a successfully enhanced island", async () => { + document.body.innerHTML = `
0
`; + const el = document.querySelector("[data-sibu-island]"); + if (!el) throw new Error("island markup missing"); + + let setupRan = false; + // The mistake: `lazyIsland(...)` omitted. This is a loader, not a setup. + const loader = () => + Promise.resolve({ + default: () => { + setupRan = true; + }, + }); + registerIsland("chart", loader as never); + + mountIslands(document.body); + await flush(); + + // The setup genuinely did not run — that part is unavoidable, the runtime + // cannot know what the function meant to be. + expect(setupRan).toBe(false); + // What must NOT happen is claiming the island is live anyway. + expect(el.getAttribute("data-sibu-enhanced")).not.toBe("true"); + expect(logged()).toContain("lazyIsland"); + }); + + it("still enhances normally when the loader IS wrapped", async () => { + const { lazyIsland } = await import("../src/platform/islands"); + document.body.innerHTML = `
0
`; + const el = document.querySelector("[data-sibu-island]"); + if (!el) throw new Error("island markup missing"); + + let setupRan = false; + registerIsland( + "chart", + lazyIsland(() => + Promise.resolve({ + default: () => { + setupRan = true; + }, + }), + ), + ); + + mountIslands(document.body); + await flush(); + + expect(setupRan).toBe(true); + expect(el.getAttribute("data-sibu-enhanced")).toBe("true"); + }); + + it("still enhances normally for an ordinary inline setup", async () => { + document.body.innerHTML = `
0
`; + const el = document.querySelector("[data-sibu-island]"); + if (!el) throw new Error("island markup missing"); + + let setupRan = false; + registerIsland("chart", () => { + setupRan = true; + }); + + mountIslands(document.body); + await flush(); + + expect(setupRan).toBe(true); + expect(el.getAttribute("data-sibu-enhanced")).toBe("true"); + }); +}); + +describe("Suspense recognises async work by shape, not by realm", () => { + it("awaits a cross-realm promise instead of rendering an error", async () => { + const { runInNewContext } = await import("node:vm"); + const resolved = el("loaded"); + // A real promise built in another realm: `instanceof Promise` is false for + // it here, but it is thenable and awaitable in every meaningful sense. + const foreign = runInNewContext("(v) => Promise.resolve(v)")(resolved) as Promise; + expect(foreign instanceof Promise).toBe(false); + + const host = div([Suspense({ nodes: () => foreign, fallback: () => el("loading") })]); + document.body.appendChild(host); + await flush(); + + expect(host.querySelector(".suspense-error")).toBeNull(); + expect(host.textContent).toContain("loaded"); + // The resolved element must be IN the document, not orphaned while live. + expect(resolved.isConnected).toBe(true); + }); + + it("awaits a plain thenable", async () => { + const resolved = el("thenable-loaded"); + const thenable = { + // biome-ignore lint/suspicious/noThenProperty: a thenable is the subject of this test + then(onFulfilled: (v: HTMLElement) => void) { + queueMicrotask(() => onFulfilled(resolved)); + }, + }; + + const host = div([Suspense({ nodes: () => thenable as never, fallback: () => el("loading") })]); + document.body.appendChild(host); + await flush(); + + expect(host.querySelector(".suspense-error")).toBeNull(); + expect(resolved.isConnected).toBe(true); + }); + + it("still renders a synchronous element without a fallback flash", async () => { + const host = div([Suspense({ nodes: () => el("sync"), fallback: () => el("loading") })]); + document.body.appendChild(host); + await flush(); + + expect(host.textContent).toContain("sync"); + expect(host.textContent).not.toContain("loading"); + }); + + it("still reports a rejected promise through the error branch", async () => { + const host = div([Suspense({ nodes: () => Promise.reject(new Error("boom")), fallback: () => el("loading") })]); + document.body.appendChild(host); + await flush(); + + expect(host.querySelector(".suspense-error")?.textContent).toBe("boom"); + }); +}); From 7236a486ebd19ab7d3f1ac4c2509a8d6a1a45935 Mon Sep 17 00:00:00 2001 From: hexplus Date: Sun, 6 Sep 2026 23:27:25 -0600 Subject: [PATCH 2/4] fix: guard thenable setups in enhance() and harden the Suspense shape check --- .tmp-probe/mod.ts | 1 - .tmp-probe/probe.ts | 12 --- CHANGELOG.md | 70 ++++++++++++++ package-lock.json | 4 +- package.json | 2 +- src/core/dev.ts | 22 ++--- src/platform/enhance.ts | 51 +++++++++- src/platform/islands.ts | 54 ++++------- src/plugins/router.ts | 19 +++- tests/dist-artifacts.test.ts | 10 ++ tests/enhance-thenable-setup.test.ts | 111 ++++++++++++++++++++++ tests/thenable-and-loader-shapes.test.ts | 45 ++++++++- tests/treeshaking-dev-diagnostics.test.ts | 1 + 13 files changed, 329 insertions(+), 73 deletions(-) delete mode 100644 .tmp-probe/mod.ts delete mode 100644 .tmp-probe/probe.ts create mode 100644 tests/enhance-thenable-setup.test.ts diff --git a/.tmp-probe/mod.ts b/.tmp-probe/mod.ts deleted file mode 100644 index b2d1314..0000000 --- a/.tmp-probe/mod.ts +++ /dev/null @@ -1 +0,0 @@ -export default (() => {}) as unknown as import("../src/platform/enhance").EnhanceSetup; diff --git a/.tmp-probe/probe.ts b/.tmp-probe/probe.ts deleted file mode 100644 index bfa098b..0000000 --- a/.tmp-probe/probe.ts +++ /dev/null @@ -1,12 +0,0 @@ -import { registerIsland, lazyIsland } from "../src/platform/islands"; -import type { EnhanceSetup } from "../src/platform/enhance"; - -// (1) The correct form. -registerIsland("ok", lazyIsland(() => import("./mod.js") as Promise<{ default: EnhanceSetup }>)); - -// (2) The mistake: a loader that was never wrapped. -registerIsland("oops", () => import("./mod.js") as Promise<{ default: EnhanceSetup }>); - -// (3) Is a zero-arg promise-returning fn assignable to EnhanceSetup directly? -const asSetup: EnhanceSetup = () => Promise.resolve(1) as unknown as Promise; -void asSetup; diff --git a/CHANGELOG.md b/CHANGELOG.md index 8282b20..e00b3fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,76 @@ This project follows [Semantic Versioning](https://semver.org/). --- --- +## [4.3.0] — 2026-09-07 + +Two defects where a value of the right *shape* was judged by the wrong test, so +the runtime confidently did the wrong thing. Minor rather than patch: one public +type is deliberately narrowed and one previously-silent case now throws. + +### Fixed + +- **An island loader that was never wrapped in `lazyIsland()` was run as a + setup.** A setup and a loader are both plain functions, so nothing could tell + them apart before one was called. Invoked as a setup, the loader ignored its + `ctx`, returned an import promise nobody awaited, and its module was never + fetched — yet `enhance()` returned normally and the element was stamped + `data-sibu-enhanced="true"`. The marker claimed an enhancement whose real + setup had never run, which is the one thing the marker exists to be trusted + about. + + The guard lives in `enhance()` rather than in `mountIslands`, because + `enhance()` and `enhanceAll()` are public and are the most direct way to reach + the same defect. A setup returning a thenable now throws — before the commit + that records ownership and sets the marker, so the transaction rolls back and + leaves the root exactly as unenhanced as it started. + +- **`Suspense` decided "is this async?" with `instanceof Promise`.** That asks + which realm built the object, not what it can do. A promise from an iframe, a + `vm` context, a worker bridge or a polyfill failed the test and was treated as + a DOM node: `insertBefore` threw, the boundary rendered its error branch for + work that was about to succeed, and the element the promise went on to resolve + to was never inserted and never disposed — live reactive bindings attached to + nothing. + + The check is now by shape (`typeof value.then === "function"`), which is what + `await` itself accepts. Nodes are excluded by a realm-agnostic `nodeType` + test, so a custom element exposing a `then` method is still inserted rather + than awaited. + +### Changed + +- **`registerIsland` no longer accepts an unwrapped loader.** `lazyIsland()` + returns a branded `LazyIslandLoader`, and `IslandRegistration` accepts only + that or an inline `EnhanceSetup`. This is the compile-time half of the fix + above: the mistake is now a type error instead of a silent runtime failure. + + **Migration:** wrap the loader — `registerIsland("chart", lazyIsland(() => + import("./chart.js")))`. Code that annotates a variable as `IslandLoader` + before passing it discards the brand and must wrap at the call site, or widen + the annotation to `LazyIslandLoader`. The brand is a phantom string-keyed + property rather than a `unique symbol`, so it stays assignable across + duplicate copies of the package in one dependency tree — the same scenario the + runtime registry already shares through `Symbol.for`. + +- **An `async` enhancement setup now throws instead of half-working.** + Previously everything before its first `await` was registered and everything + after it escaped the transaction — outside the rollback, outside the disposer, + and after the commit. It was never supported (`EnhanceSetup` returns + `void | (() => void)`); it simply failed quietly. Make the setup synchronous + and do async work inside an effect or a lifecycle hook. + +- **`Suspense`'s props match what it accepts.** `nodes` is typed + `() => HTMLElement | PromiseLike`, so the cross-realm and + thenable values the fix exists for no longer need a cast; `fallback` is typed + `(() => HTMLElement) | HTMLElement`, which the runtime already handled. + +The guard's error is thrown in production as well as development — a check that +stops a broken enhancement being reported as successful cannot be +development-only. Only its long explanation is compiled out, leaving a short +message; `tests/dist-artifacts.test.ts` asserts both halves of that. + +--- + ## [4.2.0] — 2026-09-06 Making the runtime loud where it used to be quiet. Every item below is a case diff --git a/package-lock.json b/package-lock.json index 4e60790..4f1516f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "sibujs", - "version": "4.2.0", + "version": "4.3.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "sibujs", - "version": "4.2.0", + "version": "4.3.0", "license": "MIT", "devDependencies": { "@biomejs/biome": "2.4.7", diff --git a/package.json b/package.json index 9be92d0..9948daf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "sibujs", - "version": "4.2.0", + "version": "4.3.0", "description": "A lightweight, function-based frontend framework that combines the best of React, Svelte, and Vue — with zero VDOM and maximum simplicity. Designed for developers who want fine-grained reactivity and full control without compilation or magic.", "keywords": [ "frontend", diff --git a/src/core/dev.ts b/src/core/dev.ts index bb1e2d8..8b96bc3 100644 --- a/src/core/dev.ts +++ b/src/core/dev.ts @@ -102,17 +102,6 @@ export function devAssert(condition: boolean, message: string): void { } } -/** - * Warn in dev mode only. No-op in production. - * - * Because the body is guarded by {@link DEV}, a production bundler folds this - * function to an empty one, inlines it at every call site, and drops the - * message literals with it — so a `devWarn` call costs nothing in production - * even when the call site itself is unguarded. - * - * @param message Warning text, printed to `console.warn` prefixed `[SibuJS]`. - * @returns Nothing. - */ /** * Warn in dev only, composing the message lazily. * @@ -141,6 +130,17 @@ export function devWarnLazy(build: () => string): void { } } +/** + * Warn in dev mode only. No-op in production. + * + * Because the body is guarded by {@link DEV}, a production bundler folds this + * function to an empty one, inlines it at every call site, and drops the + * message literals with it — so a `devWarn` call costs nothing in production + * even when the call site itself is unguarded. + * + * @param message Warning text, printed to `console.warn` prefixed `[SibuJS]`. + * @returns Nothing. + */ export function devWarn(message: string): void { // The `__SIBU_DEV__` test is repeated INLINE here rather than reusing `DEV`, // and that redundancy is the point. diff --git a/src/platform/enhance.ts b/src/platform/enhance.ts index 6d49a04..b3ed66b 100644 --- a/src/platform/enhance.ts +++ b/src/platform/enhance.ts @@ -13,7 +13,7 @@ // and ties every binding to disposal — so static content never re-paints. // --------------------------------------------------------------------------- -import { devAssert, isDev } from "../core/dev"; +import { DEV, devAssert, isDev } from "../core/dev"; import { MAX_DRAIN_TEARDOWNS, registerDisposer, @@ -27,6 +27,8 @@ import { setSafeAttribute } from "../utils/setSafeAttribute"; /** Attribute marking a root that *currently* owns an active enhancement. * Added on commit, removed on disposal — see the lifecycle notes on * {@link enhance}. */ +declare const __SIBU_DEV__: boolean | undefined; + const ENHANCED_ATTR = "data-sibu-enhanced"; /** @@ -551,6 +553,53 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo let extra: void | (() => void); try { extra = setup(ctx); + // A setup that returns a thenable did not finish inside this transaction, + // so treating it as successful is a lie the marker would then carry. + // + // Two distinct mistakes arrive as the same shape, and neither is + // detectable before the call — an island setup and a lazy loader are both + // plain functions: + // + // - a loader that was never wrapped in `lazyIsland()`. Invoked as a + // setup it ignores `ctx`, returns the import promise, and its module + // is never loaded, so the real setup never runs at all. + // - an `async` setup. Everything after its first `await` registers + // outside this try block: past the rollback, past the disposer, and + // past the commit below. + // + // Throwing rather than warning is what makes this safe: the commit that + // records ownership and sets `data-sibu-enhanced` is below, so an + // exception here leaves the root exactly as unenhanced as it started. + const returned: unknown = extra; + if (returned && typeof (returned as PromiseLike).then === "function") { + // The thenable is about to be discarded with nobody left to observe it. + // An unwrapped loader whose `import()` 404s would otherwise raise an + // unhandled rejection on top of the error we are throwing — and that + // rejection is the more confusing of the two, because it names a module + // the developer did not knowingly ask anyone to load. Report it and mark + // it handled. + (returned as PromiseLike).then(undefined, (reason: unknown) => { + if (typeof console !== "undefined") { + console.error("[SibuJS enhance] the promise returned by the setup also rejected:", reason); + } + }); + // The THROW ships in both builds — a guard that stops a broken + // enhancement being reported as successful cannot be development-only. + // Only the prose is traded away, via the inline define test rather than + // `DEV`: the published `dist` chunk defeats `DEV`'s inlining (see + // `src/core/dev.ts`), and a helper taking a message-building callback + // does not fold either, because its own body stays live and the argument + // is therefore still evaluated. A ternary on the define folds, and the + // dead branch's literals go with it. + throw new Error( + (typeof __SIBU_DEV__ !== "undefined" ? __SIBU_DEV__ : DEV) + ? "[SibuJS enhance] the setup returned a promise, so its work did not complete inside the enhancement " + + "transaction. If it is a lazy import, register it as lazyIsland(() => import(…)) — an unwrapped loader " + + "is called as a setup, so its module is never loaded. If it is an async setup, make it synchronous: " + + "bindings registered after an await escape both the rollback and the disposer." + : "[SibuJS enhance] setup returned a promise", + ); + } } catch (err) { drainTeardowns(teardowns, "enhance"); throw err; diff --git a/src/platform/islands.ts b/src/platform/islands.ts index 62d0a03..73be222 100644 --- a/src/platform/islands.ts +++ b/src/platform/islands.ts @@ -34,16 +34,28 @@ export type IslandLoader = () => Promise void)`, so a promise-returning function is not * assignable to it — with the unbranded loader arm gone, an unwrapped loader * no longer satisfies `IslandRegistration` at all. + * + * The brand is a phantom STRING-keyed property, never a `unique symbol`. A + * `unique symbol` has nominal identity per declaration, so two copies of this + * package's `.d.ts` in one dependency tree would produce two incompatible + * `LazyIslandLoader` types and `lazyIsland()` output from one copy would not + * satisfy the other copy's `registerIsland`. That is not hypothetical here: + * the registry is deliberately shared through `Symbol.for` precisely because + * duplicate copies are expected. A structural brand stays assignable across + * them. The property exists only in the type — the runtime marker is still the + * global symbol below. */ -export type LazyIslandLoader = IslandLoader & { readonly [LAZY]: true }; +export type LazyIslandLoader = IslandLoader & { readonly __sibujsLazyIsland: true }; /** Either an inline setup, or a {@link lazyIsland}-branded loader. */ export type IslandRegistration = EnhanceSetup | LazyIslandLoader; /** Island ids appear in attribute selectors and registry lookups. */ const SAFE_NAME = /^[A-Za-z0-9_-]+$/; -/** Brand distinguishing a lazy loader from an inline setup (both are functions). */ -const LAZY: unique symbol = Symbol.for("sibujs.islands.lazy") as never; +/** Runtime brand distinguishing a lazy loader from an inline setup (both are + * functions). Registered globally so a loader wrapped by one copy of the + * package is still recognised by another. */ +const LAZY = Symbol.for("sibujs.islands.lazy"); // Shared across duplicate runtime copies so islands registered through one copy // are mountable by mountIslands() called through another. @@ -98,40 +110,6 @@ async function resolveSetup(reg: IslandRegistration): Promise { - const returned = setup(ctx) as unknown; - if (returned && typeof (returned as PromiseLike).then === "function") { - throw new Error( - `[SibuJS islands] the setup for "${name}" returned a promise. ` + - "If it is a lazy import, register it as lazyIsland(() => import(…)) — an unwrapped loader is called as a " + - "setup, so its module is never loaded. If it is an async setup, make it synchronous: enhance() is a " + - "synchronous transaction, and bindings registered after an await escape both its rollback and its disposer.", - ); - } - return returned as ReturnType; - }; -} - export interface MountIslandsOptions { /** IntersectionObserver options for the `visible` strategy. */ rootMargin?: string; @@ -214,7 +192,7 @@ export function mountIslands( // already rolled back the island's bindings/listeners — the isolation // is real lifecycle isolation, not just control flow. try { - const disposeIsland = enhance(el, rejectThenableSetup(name, setup)); + const disposeIsland = enhance(el, setup); // Teardown can also land *during* setup (setup reaches the cleanup, // directly or via a parent). The disposers list was drained before // this disposer existed, so pushing it now would strand the island diff --git a/src/plugins/router.ts b/src/plugins/router.ts index 30df073..6c88b7c 100644 --- a/src/plugins/router.ts +++ b/src/plugins/router.ts @@ -2774,8 +2774,16 @@ export function RouterLink( * because ownership is the correct primitive to express that with. */ export function Suspense(props: { - fallback?: () => HTMLElement | HTMLElement; - nodes: () => HTMLElement | Promise; + /** Shown while `nodes` is pending. An element, or a function returning one. */ + fallback?: (() => HTMLElement) | HTMLElement; + /** + * The content. May be an element, or any thenable resolving to one — + * `PromiseLike`, not `Promise`, because a promise from another realm or a + * custom thenable is awaited just the same. The previous type admitted only + * `Promise`, so the very values the runtime was fixed to handle still had to + * be cast at the call site. + */ + nodes: () => HTMLElement | PromiseLike; }): Node { const anchor = document.createComment("suspense-boundary"); let currentNode: Node | null = null; @@ -2864,7 +2872,12 @@ export function Suspense(props: { // // `await` already accepts any thenable, so shape is both the safer test // and the one that matches what the next line actually does. - if (result != null && typeof (result as PromiseLike).then === "function") { + // A DOM node is never async work, even if it happens to expose `then`. + // Custom elements can define one, and `nodeType` is the realm-agnostic + // way to ask — `instanceof Node` would fail for a node from an iframe, + // reintroducing the very cross-realm blindness this check replaced. + const isNode = typeof (result as { nodeType?: unknown } | null)?.nodeType === "number"; + if (!isNode && result != null && typeof (result as PromiseLike).then === "function") { showFallback(myGeneration); element = await (result as PromiseLike); } else { diff --git a/tests/dist-artifacts.test.ts b/tests/dist-artifacts.test.ts index cb76914..2ee8939 100644 --- a/tests/dist-artifacts.test.ts +++ b/tests/dist-artifacts.test.ts @@ -42,6 +42,7 @@ const DIAGNOSTIC_MARKERS = { "when()/match() element-branch reuse": "branch was given as an element", "duplicate reactive runtime": "Multiple instances of the reactive runtime", "warning cap notice": "suppressing further", + "thenable-setup explanation": "an unwrapped loader is called as a setup", } as const; // `dist/` only exists after `npm run build`. Skipping locally keeps a plain @@ -89,6 +90,15 @@ describe.skipIf(!built && !onCI)("published CDN artifacts", () => { expect(prod).toBeLessThan(dev); }); + it("keeps the enhancement guard's own error message in production", () => { + // The thenable guard is behaviour, not a diagnostic: it stops a broken + // enhancement being reported as successful, so it must throw in production + // too. Only its long explanation is traded away. If this string ever + // disappears, the guard went with it. + const source = readFileSync(PROD_CDN, "utf8"); + expect(source).toContain("setup returned a promise"); + }); + it("the production CDN bundle still self-registers on window", () => { // Stripping diagnostics must not strip the entry behaviour that makes this // artifact a CDN bundle at all. diff --git a/tests/enhance-thenable-setup.test.ts b/tests/enhance-thenable-setup.test.ts new file mode 100644 index 0000000..00fe0d9 --- /dev/null +++ b/tests/enhance-thenable-setup.test.ts @@ -0,0 +1,111 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { enhance, enhanceAll } from "../src/platform/enhance"; + +// --------------------------------------------------------------------------- +// A setup that returns a thenable is rejected at the primitive, not at one +// caller. +// +// The first version of this guard lived in `mountIslands`, which meant it only +// covered islands. `enhance()` and `enhanceAll()` are public and are the most +// direct way to hit the same defect: an `async` setup registers whatever +// bindings run before its first `await`, silently abandons the rest outside the +// transaction, and — because it returns normally — gets `data-sibu-enhanced` +// stamped on the root. The marker then claims an enhancement that never +// completed, which is the whole thing the marker exists to be trusted about. +// +// `enhance()` records ownership and sets the marker only after the setup +// returns, so throwing from inside the setup rolls the transaction back and +// leaves no marker behind. +// --------------------------------------------------------------------------- + +let error: ReturnType; + +beforeEach(() => { + error = vi.spyOn(console, "error").mockImplementation(() => {}); + document.body.innerHTML = ""; +}); +afterEach(() => { + error.mockRestore(); + document.body.innerHTML = ""; +}); + +describe("enhance() rejects a setup that returns a thenable", () => { + it("throws for an async setup and leaves no enhanced marker", () => { + document.body.innerHTML = `
0
`; + const root = document.getElementById("r") as HTMLElement; + + expect(() => enhance(root, (async () => {}) as never)).toThrow(/promise/i); + expect(root.getAttribute("data-sibu-enhanced")).not.toBe("true"); + }); + + it("names both causes so the message is actionable either way", () => { + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + let message = ""; + try { + enhance(root, (() => Promise.resolve()) as never); + } catch (err) { + message = err instanceof Error ? err.message : String(err); + } + expect(message).toMatch(/lazyIsland|async/i); + }); + + it("does not leave the setup's promise unhandled", async () => { + // An unwrapped loader whose import() rejects would otherwise produce an + // unhandled rejection on top of the error we throw: the thenable is + // discarded at the moment we bail, with nobody left to observe it. + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + const unhandled: unknown[] = []; + const onUnhandled = (e: PromiseRejectionEvent | { reason?: unknown }) => + unhandled.push((e as { reason?: unknown }).reason); + process.on("unhandledRejection", onUnhandled); + + try { + expect(() => enhance(root, (() => Promise.reject(new Error("import 404"))) as never)).toThrow(); + // Give the microtask queue and the rejection callback a chance to run. + await new Promise((r) => setTimeout(r, 20)); + expect(unhandled).toHaveLength(0); + } finally { + process.off("unhandledRejection", onUnhandled); + } + }); + + it("still enhances a normal synchronous setup", () => { + document.body.innerHTML = `
0
`; + const root = document.getElementById("r") as HTMLElement; + + let ran = false; + const dispose = enhance(root, () => { + ran = true; + }); + + expect(ran).toBe(true); + expect(root.getAttribute("data-sibu-enhanced")).toBe("true"); + dispose(); + }); + + it("still honours a setup that returns a cleanup function", () => { + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + let cleaned = false; + const dispose = enhance(root, () => () => { + cleaned = true; + }); + dispose(); + expect(cleaned).toBe(true); + }); + + it("enhanceAll() rolls back rather than leaving half the collection marked", () => { + document.body.innerHTML = `
`; + const els = Array.from(document.querySelectorAll(".e")); + + expect(() => enhanceAll(".e", (async () => {}) as never)).toThrow(/promise/i); + for (const el of els) { + expect(el.getAttribute("data-sibu-enhanced")).not.toBe("true"); + } + }); +}); diff --git a/tests/thenable-and-loader-shapes.test.ts b/tests/thenable-and-loader-shapes.test.ts index eea578c..146d520 100644 --- a/tests/thenable-and-loader-shapes.test.ts +++ b/tests/thenable-and-loader-shapes.test.ts @@ -157,14 +157,15 @@ describe("Suspense recognises async work by shape, not by realm", () => { it("awaits a plain thenable", async () => { const resolved = el("thenable-loaded"); - const thenable = { + const thenable: PromiseLike = { // biome-ignore lint/suspicious/noThenProperty: a thenable is the subject of this test - then(onFulfilled: (v: HTMLElement) => void) { - queueMicrotask(() => onFulfilled(resolved)); + then(onFulfilled?: ((v: HTMLElement) => never) | null) { + queueMicrotask(() => onFulfilled?.(resolved)); + return thenable as never; }, }; - const host = div([Suspense({ nodes: () => thenable as never, fallback: () => el("loading") })]); + const host = div([Suspense({ nodes: () => thenable, fallback: () => el("loading") })]); document.body.appendChild(host); await flush(); @@ -172,6 +173,42 @@ describe("Suspense recognises async work by shape, not by realm", () => { expect(resolved.isConnected).toBe(true); }); + it("inserts a DOM node that happens to expose `then` instead of awaiting it", async () => { + // A custom element may define a `then` method. Shape alone would classify + // it as async, leaving the boundary on its fallback forever — so the check + // excludes anything with a numeric `nodeType`, which also covers a node + // from another realm where `instanceof Node` would not. + const node = el("i-am-a-node") as HTMLElement & { then?: unknown }; + // biome-ignore lint/suspicious/noThenProperty: a node with `then` is the case under test + node.then = () => { + throw new Error("Suspense awaited a DOM node"); + }; + + const host = div([Suspense({ nodes: () => node, fallback: () => el("loading") })]); + document.body.appendChild(host); + await flush(); + + expect(host.textContent).toContain("i-am-a-node"); + expect(host.querySelector(".suspense-error")).toBeNull(); + expect(node.isConnected).toBe(true); + }); + + it("accepts an element as the fallback, not only a function", async () => { + let resolveIt: (v: HTMLElement) => void = () => {}; + const pending = new Promise((r) => { + resolveIt = r; + }); + + const host = div([Suspense({ nodes: () => pending, fallback: el("waiting") })]); + document.body.appendChild(host); + await flush(); + expect(host.textContent).toContain("waiting"); + + resolveIt(el("done")); + await flush(); + expect(host.textContent).toContain("done"); + }); + it("still renders a synchronous element without a fallback flash", async () => { const host = div([Suspense({ nodes: () => el("sync"), fallback: () => el("loading") })]); document.body.appendChild(host); diff --git a/tests/treeshaking-dev-diagnostics.test.ts b/tests/treeshaking-dev-diagnostics.test.ts index 672e86a..ad916e2 100644 --- a/tests/treeshaking-dev-diagnostics.test.ts +++ b/tests/treeshaking-dev-diagnostics.test.ts @@ -49,6 +49,7 @@ const CORE_MARKERS = { // Emitted by tagFactory and the style sanitizer once their warning caches // fill up, so it belongs with the core diagnostics. "warning cap notice": "suppressing further", + "thenable-setup explanation": "an unwrapped loader is called as a setup", // Explanatory prose reached only from a warning callback. It leaked twice // while this work was in progress, because tree-shaking marks a TOP-LEVEL // binding live before the `__SIBU_DEV__` define folds its only reference From 5a5d55ef2b1f4b16730bd43d9d6348f78c440965 Mon Sep 17 00:00:00 2001 From: hexplus Date: Sun, 6 Sep 2026 23:53:18 -0600 Subject: [PATCH 3/4] fix: close the enhancement context on rollback and keep the island type unnarrowed --- CHANGELOG.md | 94 ++++++---- src/platform/enhance.ts | 44 ++++- src/platform/islands.ts | 40 +++-- src/plugins/router.ts | 7 +- tests/dist-artifacts.test.ts | 2 + tests/enhance-context-closed.test.ts | 203 ++++++++++++++++++++++ tests/thenable-and-loader-shapes.test.ts | 45 ++++- tests/treeshaking-dev-diagnostics.test.ts | 2 + 8 files changed, 369 insertions(+), 68 deletions(-) create mode 100644 tests/enhance-context-closed.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e00b3fc..b36da93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,32 +10,47 @@ This project follows [Semantic Versioning](https://semver.org/). ## [4.3.0] — 2026-09-07 Two defects where a value of the right *shape* was judged by the wrong test, so -the runtime confidently did the wrong thing. Minor rather than patch: one public -type is deliberately narrowed and one previously-silent case now throws. +the runtime confidently did the wrong thing and said nothing. Both predate 4.2: +the `instanceof Promise` check dates to the first commit, the island +registration union to the reactive-islands release. ### Fixed - **An island loader that was never wrapped in `lazyIsland()` was run as a - setup.** A setup and a loader are both plain functions, so nothing could tell - them apart before one was called. Invoked as a setup, the loader ignored its - `ctx`, returned an import promise nobody awaited, and its module was never - fetched — yet `enhance()` returned normally and the element was stamped - `data-sibu-enhanced="true"`. The marker claimed an enhancement whose real - setup had never run, which is the one thing the marker exists to be trusted - about. - - The guard lives in `enhance()` rather than in `mountIslands`, because - `enhance()` and `enhanceAll()` are public and are the most direct way to reach - the same defect. A setup returning a thenable now throws — before the commit - that records ownership and sets the marker, so the transaction rolls back and - leaves the root exactly as unenhanced as it started. + setup.** A setup and a loader are both plain functions, so nothing can tell + them apart before one is called. Invoked as a setup, the loader ignored its + `ctx` and returned the import promise. The module *was* fetched — `import()` + ran — but nobody awaited it, so the setup it resolved to was discarded and + never ran. `enhance()` then returned normally and the element was stamped + `data-sibu-enhanced="true"`: a marker asserting an enhancement whose real + setup had never executed. + + The guard lives in `enhance()`, not in `mountIslands`, because `enhance()` and + `enhanceAll()` are public and reach the same defect directly. A setup + returning a thenable now throws *before* the commit that records ownership and + sets the marker, so the transaction rolls back and the root is left exactly as + unenhanced as it started. + +- **A rolled-back enhancement could still be mutated afterwards.** Detecting the + thenable and unwinding was only half of it: the async setup keeps running + after its first `await`, still holding `ctx`, and could register listeners, + bindings and cleanups into an enhancement that no longer existed. The root + carried no marker and the disposer had already drained, so those registrations + could never be released. A setup that queued a microtask and then threw + synchronously escaped the same way. + + The context is now closed once its transaction unwinds, and every mutating + method refuses afterwards with a dev warning rather than dropping the call in + silence. Closing happens *after* the teardowns drain, because a teardown may + legitimately register another cleanup while unwinding — documented behaviour + that still works. Disposal closes the context too. - **`Suspense` decided "is this async?" with `instanceof Promise`.** That asks which realm built the object, not what it can do. A promise from an iframe, a `vm` context, a worker bridge or a polyfill failed the test and was treated as a DOM node: `insertBefore` threw, the boundary rendered its error branch for - work that was about to succeed, and the element the promise went on to resolve - to was never inserted and never disposed — live reactive bindings attached to + work that was about to succeed, and the element the promise resolved to was + never inserted and never disposed — live reactive bindings attached to nothing. The check is now by shape (`typeof value.then === "function"`), which is what @@ -43,37 +58,42 @@ type is deliberately narrowed and one previously-silent case now throws. test, so a custom element exposing a `then` method is still inserted rather than awaited. -### Changed - -- **`registerIsland` no longer accepts an unwrapped loader.** `lazyIsland()` - returns a branded `LazyIslandLoader`, and `IslandRegistration` accepts only - that or an inline `EnhanceSetup`. This is the compile-time half of the fix - above: the mistake is now a type error instead of a silent runtime failure. +- **`Suspense` dropped a fallback element from another realm.** The async check + was made realm-agnostic; the fallback check was not, so `instanceof + HTMLElement` silently discarded it and the boundary rendered nothing at all + while its promise stayed pending. Both now use the same `nodeType` test. - **Migration:** wrap the loader — `registerIsland("chart", lazyIsland(() => - import("./chart.js")))`. Code that annotates a variable as `IslandLoader` - before passing it discards the brand and must wrap at the call site, or widen - the annotation to `LazyIslandLoader`. The brand is a phantom string-keyed - property rather than a `unique symbol`, so it stays assignable across - duplicate copies of the package in one dependency tree — the same scenario the - runtime registry already shares through `Symbol.for`. +### Changed - **An `async` enhancement setup now throws instead of half-working.** Previously everything before its first `await` was registered and everything - after it escaped the transaction — outside the rollback, outside the disposer, - and after the commit. It was never supported (`EnhanceSetup` returns - `void | (() => void)`); it simply failed quietly. Make the setup synchronous - and do async work inside an effect or a lifecycle hook. + after it escaped the transaction. It was never supported — `EnhanceSetup` + returns `void | (() => void)` — it simply failed quietly. Make the setup + synchronous and do async work inside an effect or a lifecycle hook. - **`Suspense`'s props match what it accepts.** `nodes` is typed `() => HTMLElement | PromiseLike`, so the cross-realm and thenable values the fix exists for no longer need a cast; `fallback` is typed `(() => HTMLElement) | HTMLElement`, which the runtime already handled. -The guard's error is thrown in production as well as development — a check that -stops a broken enhancement being reported as successful cannot be +### Added + +- **`LazyIslandLoader`** — the branded type `lazyIsland()` returns, exported for + callers that want to be explicit. + + `IslandRegistration` deliberately still accepts an *unbranded* loader. + Requiring the brand would catch a forgotten `lazyIsland(...)` at compile time, + which is where a mistake is cheapest to find, but it rejects code that + compiles today — and this package's contract is that existing public API keeps + working, with a codemod for anything that cannot be widened. There is no + codemod infrastructure to ship one through, so the narrowing is not taken and + the runtime guard carries the fix instead. A test records that decision, so a + later tightening cannot happen by accident. + +The enhancement guard's error is thrown in production as well as development — a +check that stops a broken enhancement being reported as successful cannot be development-only. Only its long explanation is compiled out, leaving a short -message; `tests/dist-artifacts.test.ts` asserts both halves of that. +message; `tests/dist-artifacts.test.ts` asserts both halves. --- diff --git a/src/platform/enhance.ts b/src/platform/enhance.ts index b3ed66b..7a98d4e 100644 --- a/src/platform/enhance.ts +++ b/src/platform/enhance.ts @@ -13,7 +13,7 @@ // and ties every binding to disposal — so static content never re-paints. // --------------------------------------------------------------------------- -import { DEV, devAssert, isDev } from "../core/dev"; +import { DEV, devAssert, devWarnLazy, isDev } from "../core/dev"; import { MAX_DRAIN_TEARDOWNS, registerDisposer, @@ -400,7 +400,33 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo const owner = Symbol("sibujs.enhance"); let disposed = false; + // Whether this context's transaction has been unwound. A setup that returned + // a thenable — or one that queued a microtask and then threw — keeps running + // after `enhance()` has rolled everything back, and it still holds `ctx`. + // Without this flag those continuations register listeners, bindings and + // cleanups into an enhancement nobody owns: the root carries no marker, the + // disposer has already drained, and the registration can never be released. + // + // Set AFTER the teardowns drain, never before: a teardown may legitimately + // call `ctx.cleanup` while unwinding, and the drain loops until the list is + // stable. Closing early would break that documented reentrancy. + let closed = false; + + /** True when the context is dead; warns in dev so the drop is not silent. */ + const isClosed = (method: string): boolean => { + if (!closed) return false; + devWarnLazy( + () => + `enhance: ctx.${method}() was called after this enhancement was rolled back or disposed, so it was ignored. ` + + "The setup is still running past the point where its transaction ended — usually an async setup continuing " + + "after an await, or a callback it queued before it threw. Registrations made now would belong to nothing: " + + "the root carries no enhancement marker and the disposer has already run, so nothing could ever release them.", + ); + return true; + }; + const bind = (target_: string | Element | null, fn: (el: HTMLElement) => void): void => { + if (isClosed("bind")) return; const el = resolveTarget(root, target_); if (!el) { if (typeof console !== "undefined") { @@ -526,6 +552,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo }); }, each: (target_, describe) => { + if (isClosed("each")) return; devAssert(typeof describe === "function", "ctx.each: second argument must be a function."); const elements = typeof target_ === "string" ? ctx.refs(target_) : (Array.from(target_) as HTMLElement[]); @@ -539,6 +566,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo } }, cleanup: (fn) => { + if (isClosed("cleanup")) return; teardowns.push(fn); }, }; @@ -561,8 +589,9 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo // plain functions: // // - a loader that was never wrapped in `lazyIsland()`. Invoked as a - // setup it ignores `ctx`, returns the import promise, and its module - // is never loaded, so the real setup never runs at all. + // setup it ignores `ctx` and returns the import promise. The module IS + // fetched — `import()` ran — but nobody awaits it, so the setup it + // resolves to is discarded and never runs. // - an `async` setup. Everything after its first `await` registers // outside this try block: past the rollback, past the disposer, and // past the commit below. @@ -579,9 +608,10 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo // the developer did not knowingly ask anyone to load. Report it and mark // it handled. (returned as PromiseLike).then(undefined, (reason: unknown) => { - if (typeof console !== "undefined") { - console.error("[SibuJS enhance] the promise returned by the setup also rejected:", reason); - } + // The handler itself must exist in every build — its job is to mark the + // rejection handled — but the explanation is a diagnostic and compiles + // out with the rest of them. + devWarnLazy(() => `enhance: the promise returned by the setup also rejected: ${String(reason)}`); }); // The THROW ships in both builds — a guard that stops a broken // enhancement being reported as successful cannot be development-only. @@ -602,6 +632,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo } } catch (err) { drainTeardowns(teardowns, "enhance"); + closed = true; throw err; } if (typeof extra === "function") teardowns.push(extra); @@ -619,6 +650,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo // enhance/dispose cycles on a long-lived root don't accumulate closures. unregisterDisposer(root, dispose); drainTeardowns(teardowns, "enhance"); + closed = true; }; // Commit. Ownership is recorded and only *then* is the root marked, so the diff --git a/src/platform/islands.ts b/src/platform/islands.ts index 73be222..3259cfa 100644 --- a/src/platform/islands.ts +++ b/src/platform/islands.ts @@ -23,32 +23,36 @@ export type IslandLoader = () => Promise void)`, so a promise-returning function is not - * assignable to it — with the unbranded loader arm gone, an unwrapped loader - * no longer satisfies `IslandRegistration` at all. + * Requiring the brand would catch a forgotten `lazyIsland(...)` at compile time, + * which is where a mistake is cheapest to find — but it also rejects code that + * compiles today, and this package's contract is that the existing public API + * keeps working, widening rather than replacing, with a codemod for anything + * that cannot be widened. There is no codemod infrastructure here to ship one + * through, so the narrowing is not taken: the mistake is caught at mount time + * instead, by the thenable guard in `enhance()`, which throws immediately with + * an actionable message and leaves the element unenhanced. Loud and instant at + * runtime beats a type error that arrives with a breaking change attached. * * The brand is a phantom STRING-keyed property, never a `unique symbol`. A * `unique symbol` has nominal identity per declaration, so two copies of this * package's `.d.ts` in one dependency tree would produce two incompatible - * `LazyIslandLoader` types and `lazyIsland()` output from one copy would not - * satisfy the other copy's `registerIsland`. That is not hypothetical here: - * the registry is deliberately shared through `Symbol.for` precisely because - * duplicate copies are expected. A structural brand stays assignable across - * them. The property exists only in the type — the runtime marker is still the - * global symbol below. + * types. That is not hypothetical: the registry is deliberately shared through + * `Symbol.for` precisely because duplicate copies are expected. The property + * exists only in the type — the runtime marker is the global symbol below. */ export type LazyIslandLoader = IslandLoader & { readonly __sibujsLazyIsland: true }; -/** Either an inline setup, or a {@link lazyIsland}-branded loader. */ -export type IslandRegistration = EnhanceSetup | LazyIslandLoader; +/** + * Either an inline setup, or a loader. + * + * Unchanged from 4.2: an unbranded `IslandLoader` is still accepted, so no + * previously-compiling code breaks. Wrapping with {@link lazyIsland} is what + * makes it *work* — see {@link LazyIslandLoader} for why this is not narrowed. + */ +export type IslandRegistration = EnhanceSetup | IslandLoader | LazyIslandLoader; /** Island ids appear in attribute selectors and registry lookups. */ const SAFE_NAME = /^[A-Za-z0-9_-]+$/; diff --git a/src/plugins/router.ts b/src/plugins/router.ts index 6c88b7c..b5c375c 100644 --- a/src/plugins/router.ts +++ b/src/plugins/router.ts @@ -2840,7 +2840,12 @@ export function Suspense(props: { return; } - if (!(fallback instanceof HTMLElement)) return; + // Realm-agnostic, for the same reason the async check below is: an element + // from an iframe or another jsdom realm fails `instanceof HTMLElement`, and + // the fallback was then dropped in silence while the promise stayed + // pending — a boundary showing nothing at all. `nodeType === 1` is what + // "is an element" actually means, in any realm. + if (!fallback || (fallback as unknown as Node).nodeType !== 1) return; const parent = commitTarget(myGeneration); if (!parent) { diff --git a/tests/dist-artifacts.test.ts b/tests/dist-artifacts.test.ts index 2ee8939..518a552 100644 --- a/tests/dist-artifacts.test.ts +++ b/tests/dist-artifacts.test.ts @@ -43,6 +43,8 @@ const DIAGNOSTIC_MARKERS = { "duplicate reactive runtime": "Multiple instances of the reactive runtime", "warning cap notice": "suppressing further", "thenable-setup explanation": "an unwrapped loader is called as a setup", + "post-rollback context use": "was called after this enhancement was rolled back", + "setup rejection explanation": "the promise returned by the setup also rejected", } as const; // `dist/` only exists after `npm run build`. Skipping locally keeps a plain diff --git a/tests/enhance-context-closed.test.ts b/tests/enhance-context-closed.test.ts new file mode 100644 index 0000000..750cbee --- /dev/null +++ b/tests/enhance-context-closed.test.ts @@ -0,0 +1,203 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { enhance } from "../src/platform/enhance"; + +// --------------------------------------------------------------------------- +// A rolled-back enhancement must stay rolled back. +// +// Detecting the thenable, draining the teardowns and throwing is only half of +// it: the async function that returned that thenable KEEPS RUNNING. Everything +// after its first `await` still holds a live `ctx`, so it can register +// listeners, bindings and cleanups into an enhancement that was already +// unwound — past the rollback, past the disposer, and with no marker on the +// root to show anything owns them. The listener works; nothing can ever remove +// it. +// +// The same escape exists without any promise at all: a setup that queues a +// microtask and then throws synchronously is unwound the same way, and its +// continuation holds the same `ctx`. +// +// The context is therefore CLOSED once its transaction has been unwound, and +// closing happens *after* the teardowns drain — a teardown may legitimately +// register another cleanup while unwinding, which is documented behaviour and +// must keep working. +// --------------------------------------------------------------------------- + +let warn: ReturnType; +let error: ReturnType; + +beforeEach(() => { + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + error = vi.spyOn(console, "error").mockImplementation(() => {}); + document.body.innerHTML = ""; +}); +afterEach(() => { + warn.mockRestore(); + error.mockRestore(); + document.body.innerHTML = ""; +}); + +const flush = async () => { + for (let i = 0; i < 10; i++) await Promise.resolve(); +}; + +describe("a rolled-back enhancement refuses later registrations", () => { + it("ignores ctx.on() called after the await", async () => { + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + let clicks = 0; + expect(() => + enhance(root, (async (ctx: import("../src/platform/enhance").EnhanceContext) => { + await Promise.resolve(); + ctx.on(root, "click", () => { + clicks++; + }); + }) as never), + ).toThrow(); + + await flush(); + root.click(); + + expect(root.getAttribute("data-sibu-enhanced")).not.toBe("true"); + expect(clicks).toBe(0); + }); + + it("ignores every mutating context method called after the await", async () => { + document.body.innerHTML = `
server
`; + const root = document.getElementById("r") as HTMLElement; + const target = root.querySelector("[data-ref=t]"); + if (!target) throw new Error("fixture missing"); + + let clicks = 0; + let cleanupRan = false; + let eachRan = false; + + expect(() => + enhance(root, (async (ctx: import("../src/platform/enhance").EnhanceContext) => { + await Promise.resolve(); + ctx.on(root, "click", () => { + clicks++; + }); + ctx.text("@t", () => "rewritten"); + ctx.attr("@t", "data-x", () => "1"); + ctx.classed("@t", "on", () => true); + ctx.show("@t", () => false); + ctx.each("@t", () => { + eachRan = true; + return { text: () => "each" }; + }); + ctx.cleanup(() => { + cleanupRan = true; + }); + }) as never), + ).toThrow(); + + await flush(); + root.click(); + + expect(clicks).toBe(0); + expect(eachRan).toBe(false); + expect(cleanupRan).toBe(false); + // The server's DOM is untouched: no text, attribute, class or display write + // landed from the continuation. + expect(target.textContent).toBe("server"); + expect(target.getAttribute("data-x")).toBeNull(); + expect(target.classList.contains("on")).toBe(false); + expect(target.hidden).toBe(false); + }); + + it("ignores a queued microtask after a synchronous setup failure", async () => { + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + let clicks = 0; + expect(() => + enhance(root, (ctx) => { + queueMicrotask(() => { + ctx.on(root, "click", () => { + clicks++; + }); + }); + throw new Error("setup failed"); + }), + ).toThrow("setup failed"); + + await flush(); + root.click(); + expect(clicks).toBe(0); + }); + + it("says so in dev rather than dropping the registration silently", async () => { + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + expect(() => + enhance(root, (async (ctx: import("../src/platform/enhance").EnhanceContext) => { + await Promise.resolve(); + ctx.on(root, "click", () => {}); + }) as never), + ).toThrow(); + + await flush(); + const messages = warn.mock.calls.map((c) => String(c[0])).join("\n"); + expect(messages).toContain("rolled back"); + }); + + it("still lets a teardown register another cleanup while unwinding", () => { + // Documented behaviour: `ctx.cleanup` stays reachable from inside a + // teardown, and the drain keeps going until the list is stable. Closing the + // context must not happen until that drain is finished. + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + let nested = false; + expect(() => + enhance(root, (ctx) => { + ctx.cleanup(() => { + ctx.cleanup(() => { + nested = true; + }); + }); + throw new Error("boom"); + }), + ).toThrow("boom"); + + expect(nested).toBe(true); + }); + + it("refuses registrations after an ordinary dispose() too", async () => { + document.body.innerHTML = `
`; + const root = document.getElementById("r") as HTMLElement; + + let captured: import("../src/platform/enhance").EnhanceContext | null = null; + let clicks = 0; + const dispose = enhance(root, (ctx) => { + captured = ctx; + }); + dispose(); + + (captured as unknown as import("../src/platform/enhance").EnhanceContext).on(root, "click", () => { + clicks++; + }); + root.click(); + expect(clicks).toBe(0); + }); + + it("does not interfere with a healthy enhancement", () => { + document.body.innerHTML = `
0
`; + const root = document.getElementById("r") as HTMLElement; + + let clicks = 0; + const dispose = enhance(root, (ctx) => { + ctx.on(root, "click", () => { + clicks++; + }); + ctx.text("@t", () => "live"); + }); + + root.click(); + expect(clicks).toBe(1); + expect(root.querySelector("[data-ref=t]")?.textContent).toBe("live"); + dispose(); + }); +}); diff --git a/tests/thenable-and-loader-shapes.test.ts b/tests/thenable-and-loader-shapes.test.ts index 146d520..d28fbc2 100644 --- a/tests/thenable-and-loader-shapes.test.ts +++ b/tests/thenable-and-loader-shapes.test.ts @@ -47,18 +47,21 @@ const el = (...args: Parameters): HTMLElement => div(...args) as HTM const logged = () => [...warn.mock.calls, ...error.mock.calls].map((c) => c.map((a) => String(a)).join(" ")).join("\n"); /** - * Compile-time half of the island fix, checked by `npm run typecheck:tests`. + * Records that an unwrapped loader still COMPILES, deliberately. * - * Never executed — its value is that `tsc` fails if the `@ts-expect-error` - * stops being an error, i.e. if `IslandRegistration` ever readmits an unbranded - * loader. The runtime tests below cover JS callers and casts that bypass this. + * Requiring `lazyIsland()` at the type level would catch the mistake earlier, + * but it rejects code that compiles today and there is no codemod to carry + * callers across — so the contract is left alone and the runtime guard in + * `enhance()` is what catches it, immediately and loudly. If this ever starts + * failing to compile, the public type was narrowed and that needs a codemod and + * a major version, not a silent tightening. + * + * Never executed; checked by `npm run typecheck:tests`. */ function _islandRegistrationTypes(): void { const loader = () => Promise.resolve({ default: () => {} }); - // @ts-expect-error — an unwrapped loader is not a valid registration registerIsland("compile-unwrapped", loader); - registerIsland("compile-wrapped", lazyIsland(loader)); registerIsland("compile-inline", () => {}); } @@ -209,6 +212,36 @@ describe("Suspense recognises async work by shape, not by realm", () => { expect(host.textContent).toContain("done"); }); + it("shows a fallback element built in another realm", async () => { + // The async check was made realm-agnostic first; the fallback check was + // not, so an element from an iframe or a second jsdom failed + // `instanceof HTMLElement` and was dropped in silence — the boundary + // rendered nothing at all while its promise stayed pending. + // @ts-expect-error — jsdom ships no types here; this is a test-only import. + const { JSDOM } = await import("jsdom"); + const other = new JSDOM(""); + const foreignFallback = other.window.document.createElement("div"); + foreignFallback.textContent = "foreign-loading"; + expect(foreignFallback instanceof HTMLElement).toBe(false); + expect(foreignFallback.nodeType).toBe(1); + + let resolveIt: (v: HTMLElement) => void = () => {}; + const pending = new Promise((r) => { + resolveIt = r; + }); + + const host = div([Suspense({ nodes: () => pending, fallback: foreignFallback as unknown as HTMLElement })]); + document.body.appendChild(host); + await flush(); + + expect(host.textContent).toContain("foreign-loading"); + + resolveIt(el("done")); + await flush(); + expect(host.textContent).toContain("done"); + expect(host.textContent).not.toContain("foreign-loading"); + }); + it("still renders a synchronous element without a fallback flash", async () => { const host = div([Suspense({ nodes: () => el("sync"), fallback: () => el("loading") })]); document.body.appendChild(host); diff --git a/tests/treeshaking-dev-diagnostics.test.ts b/tests/treeshaking-dev-diagnostics.test.ts index ad916e2..0be10a4 100644 --- a/tests/treeshaking-dev-diagnostics.test.ts +++ b/tests/treeshaking-dev-diagnostics.test.ts @@ -50,6 +50,8 @@ const CORE_MARKERS = { // fill up, so it belongs with the core diagnostics. "warning cap notice": "suppressing further", "thenable-setup explanation": "an unwrapped loader is called as a setup", + "post-rollback context use": "was called after this enhancement was rolled back", + "setup rejection explanation": "the promise returned by the setup also rejected", // Explanatory prose reached only from a warning callback. It leaked twice // while this work was in progress, because tree-shaking marks a TOP-LEVEL // binding live before the `__SIBU_DEV__` define folds its only reference From bc4fe28398e5e6e03a6aaf359473a31eb2e37f48 Mon Sep 17 00:00:00 2001 From: hexplus Date: Mon, 7 Sep 2026 00:04:28 -0600 Subject: [PATCH 4/4] fix: correct the stale enhancement error text and name the real ctx method --- src/platform/enhance.ts | 23 ++++++++++++----------- tests/enhance-context-closed.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/platform/enhance.ts b/src/platform/enhance.ts index 7a98d4e..dbfe82f 100644 --- a/src/platform/enhance.ts +++ b/src/platform/enhance.ts @@ -425,8 +425,8 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo return true; }; - const bind = (target_: string | Element | null, fn: (el: HTMLElement) => void): void => { - if (isClosed("bind")) return; + const bind = (method: string, target_: string | Element | null, fn: (el: HTMLElement) => void): void => { + if (isClosed(method)) return; const el = resolveTarget(root, target_); if (!el) { if (typeof console !== "undefined") { @@ -449,14 +449,14 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo } }, on: (t, event, handler, options) => { - bind(t, (el) => { + bind("on", t, (el) => { const wrapped = (e: Event) => handler(e as never, el); el.addEventListener(event, wrapped as EventListener, options); teardowns.push(() => el.removeEventListener(event, wrapped as EventListener, options)); }); }, text: (t, value) => { - bind(t, (el) => { + bind("text", t, (el) => { teardowns.push( bindNode(el, () => { const v = value(); @@ -470,7 +470,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo }); }, attr: (t, name, value) => { - bind(t, (el) => { + bind("attr", t, (el) => { teardowns.push( bindNode(el, () => { const v = value(); @@ -504,7 +504,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo }); }, classed: (t, name, on) => { - bind(t, (el) => { + bind("classed", t, (el) => { teardowns.push( bindNode(el, () => { el.classList.toggle(name, Boolean(on())); @@ -513,7 +513,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo }); }, show: (t, when) => { - bind(t, (el) => { + bind("show", t, (el) => { // Toggle the standard `hidden` property — this both reveals an element // the server rendered with the `hidden` attribute (the common // progressive-enhancement case) and hides one that wasn't. Using @@ -530,7 +530,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo }); }, model: (t, state, options) => { - bind(t, (el) => { + bind("model", t, (el) => { const control = el as HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement; const [get, set] = state; const evt = @@ -624,9 +624,10 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo throw new Error( (typeof __SIBU_DEV__ !== "undefined" ? __SIBU_DEV__ : DEV) ? "[SibuJS enhance] the setup returned a promise, so its work did not complete inside the enhancement " + - "transaction. If it is a lazy import, register it as lazyIsland(() => import(…)) — an unwrapped loader " + - "is called as a setup, so its module is never loaded. If it is an async setup, make it synchronous: " + - "bindings registered after an await escape both the rollback and the disposer." + "transaction. If it is a lazy import, register it as lazyIsland(() => import(…)): an unwrapped loader " + + "is called as a setup, so its module is fetched but the setup it resolves to is discarded. If it is " + + "an async setup, make it synchronous — this enhancement has been rolled back, and ctx registrations " + + "made after this point are ignored." : "[SibuJS enhance] setup returned a promise", ); } diff --git a/tests/enhance-context-closed.test.ts b/tests/enhance-context-closed.test.ts index 750cbee..36482d8 100644 --- a/tests/enhance-context-closed.test.ts +++ b/tests/enhance-context-closed.test.ts @@ -127,6 +127,30 @@ describe("a rolled-back enhancement refuses later registrations", () => { expect(clicks).toBe(0); }); + it("names the public method the consumer called, not the private plumbing", async () => { + // Every binding helper routes through one internal `bind()`. Reporting that + // name would send the reader looking for a `ctx.bind()` that does not + // exist, so the caller's own name is threaded through. + document.body.innerHTML = `
server
`; + const root = document.getElementById("r") as HTMLElement; + + expect(() => + enhance(root, (async (ctx: import("../src/platform/enhance").EnhanceContext) => { + await Promise.resolve(); + ctx.text("@t", () => "x"); + ctx.attr("@t", "data-y", () => "1"); + ctx.cleanup(() => {}); + }) as never), + ).toThrow(); + + await flush(); + const messages = warn.mock.calls.map((c) => String(c[0])).join("\n"); + expect(messages).toContain("ctx.text()"); + expect(messages).toContain("ctx.attr()"); + expect(messages).toContain("ctx.cleanup()"); + expect(messages).not.toContain("ctx.bind()"); + }); + it("says so in dev rather than dropping the registration silently", async () => { document.body.innerHTML = `
`; const root = document.getElementById("r") as HTMLElement;