diff --git a/CHANGELOG.md b/CHANGELOG.md index 8282b20..b36da93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,96 @@ 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 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 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 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 + `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. + +- **`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. + +### 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. 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. + +### 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. + +--- + ## [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..dbfe82f 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, devWarnLazy, 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"; /** @@ -398,7 +400,33 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo const owner = Symbol("sibujs.enhance"); let disposed = false; - const bind = (target_: string | Element | null, fn: (el: HTMLElement) => void): void => { + // 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 = (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") { @@ -421,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(); @@ -442,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(); @@ -476,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())); @@ -485,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 @@ -502,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 = @@ -524,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[]); @@ -537,6 +566,7 @@ export function enhance(target: Element | string, setup: EnhanceSetup): () => vo } }, cleanup: (fn) => { + if (isClosed("cleanup")) return; teardowns.push(fn); }, }; @@ -551,8 +581,59 @@ 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` 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. + // + // 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) => { + // 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. + // 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 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", + ); + } } catch (err) { drainTeardowns(teardowns, "enhance"); + closed = true; throw err; } if (typeof extra === "function") teardowns.push(extra); @@ -570,6 +651,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 8a0a716..3259cfa 100644 --- a/src/platform/islands.ts +++ b/src/platform/islands.ts @@ -20,12 +20,45 @@ export type IslandStrategy = "load" | "idle" | "visible" | "interaction" | "medi * is one). Only fetched when the island activates. Wrap with {@link lazyIsland}. */ export type IslandLoader = () => Promise; -/** Either an inline setup, or a {@link lazyIsland}-branded loader. */ -export type IslandRegistration = EnhanceSetup | IslandLoader; +/** + * A loader that has been through {@link lazyIsland}. + * + * Exported for callers that want to be explicit about having wrapped one. It is + * NOT required by {@link IslandRegistration}, and that is deliberate. + * + * 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 + * 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 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_-]+$/; -/** Brand distinguishing a lazy loader from an inline setup (both are functions). */ +/** 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 @@ -41,9 +74,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; } /** diff --git a/src/plugins/router.ts b/src/plugins/router.ts index 86ac9f9..b5c375c 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; @@ -2832,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) { @@ -2852,11 +2865,28 @@ 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. + // 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; + 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/dist-artifacts.test.ts b/tests/dist-artifacts.test.ts index cb76914..518a552 100644 --- a/tests/dist-artifacts.test.ts +++ b/tests/dist-artifacts.test.ts @@ -42,6 +42,9 @@ 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", + "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 @@ -89,6 +92,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-context-closed.test.ts b/tests/enhance-context-closed.test.ts new file mode 100644 index 0000000..36482d8 --- /dev/null +++ b/tests/enhance-context-closed.test.ts @@ -0,0 +1,227 @@ +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("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; + + 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/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 new file mode 100644 index 0000000..d28fbc2 --- /dev/null +++ b/tests/thenable-and-loader-shapes.test.ts @@ -0,0 +1,261 @@ +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"); + +/** + * Records that an unwrapped loader still COMPILES, deliberately. + * + * 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: () => {} }); + + 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: PromiseLike = { + // biome-ignore lint/suspicious/noThenProperty: a thenable is the subject of this test + then(onFulfilled?: ((v: HTMLElement) => never) | null) { + queueMicrotask(() => onFulfilled?.(resolved)); + return thenable as never; + }, + }; + + const host = div([Suspense({ nodes: () => thenable, fallback: () => el("loading") })]); + document.body.appendChild(host); + await flush(); + + expect(host.querySelector(".suspense-error")).toBeNull(); + 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("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); + 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"); + }); +}); diff --git a/tests/treeshaking-dev-diagnostics.test.ts b/tests/treeshaking-dev-diagnostics.test.ts index 672e86a..0be10a4 100644 --- a/tests/treeshaking-dev-diagnostics.test.ts +++ b/tests/treeshaking-dev-diagnostics.test.ts @@ -49,6 +49,9 @@ 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", + "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