From 95628d9c27b7bf28c0603688c6d867fa7dfb9fba Mon Sep 17 00:00:00 2001 From: hexplus Date: Sun, 6 Sep 2026 21:07:19 -0600 Subject: [PATCH 1/5] fix: make silent failures observable in dev and strip diagnostics from production --- src/core/dev.ts | 151 ++++++++++-- src/core/errors.ts | 8 +- src/core/rendering/context.ts | 8 +- src/core/rendering/directives.ts | 194 ++++++++++++--- src/core/rendering/dispose.ts | 18 +- src/core/rendering/each.ts | 6 +- src/core/rendering/focusPreservation.ts | 238 ++++++++++++++++++ src/core/rendering/htm.ts | 9 +- src/core/rendering/lazy.ts | 21 ++ src/core/rendering/mount.ts | 5 + src/core/rendering/slots.ts | 7 + src/core/rendering/tagFactory.ts | 45 +++- src/core/signals/array.ts | 8 + src/core/signals/asyncDerived.ts | 3 + src/core/signals/derived.ts | 3 + src/core/signals/effect.ts | 3 + src/core/signals/ref.ts | 7 + src/core/signals/signal.ts | 17 +- src/core/signals/writable.ts | 2 + src/devtools/debug.ts | 7 + src/patterns/persist.ts | 8 + src/performance/domRecycler.ts | 6 +- src/platform/ssr.ts | 20 +- src/plugins/router.ts | 104 +++++++- src/reactivity/bindChildNode.ts | 21 +- src/reactivity/track-core.ts | 54 ++++ src/reactivity/track.ts | 17 +- src/ui/a11y.ts | 7 + src/ui/dialog.ts | 6 + src/ui/form.ts | 56 +++++ src/ui/scrollLock.ts | 6 + src/ui/toast.ts | 8 + src/utils/sanitize.ts | 212 ++++++++++++++-- src/utils/setSafeAttribute.ts | 10 +- src/widgets/Select.ts | 8 + tests/api-uniformity.test.ts | 161 ++++++++++++ tests/focus-preservation.test.ts | 181 ++++++++++++++ tests/hardening-review-regressions.test.ts | 271 +++++++++++++++++++++ tests/lone-string-heuristic-rate.test.ts | 245 +++++++++++++++++++ tests/sanitize-style-drop-warning.test.ts | 87 +++++++ tests/treeshaking-dev-diagnostics.test.ts | 105 ++++++++ 41 files changed, 2207 insertions(+), 146 deletions(-) create mode 100644 src/core/rendering/focusPreservation.ts create mode 100644 tests/api-uniformity.test.ts create mode 100644 tests/focus-preservation.test.ts create mode 100644 tests/hardening-review-regressions.test.ts create mode 100644 tests/lone-string-heuristic-rate.test.ts create mode 100644 tests/sanitize-style-drop-warning.test.ts create mode 100644 tests/treeshaking-dev-diagnostics.test.ts diff --git a/src/core/dev.ts b/src/core/dev.ts index 1b30bbc..bb1e2d8 100644 --- a/src/core/dev.ts +++ b/src/core/dev.ts @@ -6,43 +6,162 @@ * * In production: dead code elimination removes all dev checks entirely. * In development: provides clear, actionable error messages. + * + * `tests/treeshaking-dev-diagnostics.test.ts` bundles this for real and asserts + * the message strings are gone, so a regression here fails the suite rather + * than quietly shipping every warning to every consumer. */ declare const __SIBU_DEV__: boolean | undefined; /** - * Returns true when running in development mode. - * Tree-shakes to `false` in production builds. + * Dev mode as a **statically foldable constant**, snapshotted at module load. + * + * Prefer this over {@link isDev} for any guard whose only job is to gate a + * diagnostic. The distinction is not stylistic — it decides whether the + * diagnostic exists in a consumer's production bundle: + * + * - `DEV` inlines its `__SIBU_DEV__` define into every reference, so + * `if (DEV) …` becomes `if (false) …` and the whole branch — warning strings + * included — is deleted. + * - `isDev()` is a function CALL. Bundlers do not inline calls across modules, + * so `const _isDev = isDev()` produces a runtime `var` and every branch that + * reads it survives minification with its message strings intact. That alias + * is exactly why this library used to ship every warning it could emit. + * + * Use {@link isDev} only where the answer must be read LIVE at call time rather + * than snapshotted at import time (devtools opt-in defaults, `strict()`). + * + * THREE THINGS ABOUT THIS DECLARATION ARE LOAD-BEARING. Changing any of them + * silently un-strips every diagnostic in the library: + * + * 1. **It must be the first declaration in this module.** esbuild inlines a + * cross-module `const` only when no hoisted declaration precedes it; + * anything above it (a function, another const) makes the inliner give up + * and emit a runtime `var` instead. Verified empirically — moving `isDev` + * above this line is enough to put every warning string back in the bundle. + * 2. **It must be an inline expression, not `isDev()`.** A cross-module call is + * opaque to the bundler, which is the problem this constant exists to solve. + * 3. **The bare `__SIBU_DEV__` must come first in the ladder.** Only a bare + * identifier is a `define` target; `globalThis.__SIBU_DEV__` is a member + * expression and can never be substituted or folded. + * + * With no define at all (raw ESM, the test runner) an unqualified + * `__SIBU_DEV__` resolves to the global of that name, so the first branch reads + * the same value the `globalThis` branch would — which is why it coerces with + * `!!`: a test may set the override to `1` or `0` and still expect a strict + * boolean back. The runtime escape hatch `tests/prod-mode.test.ts` relies on + * keeps working, and diagnostics correctly stay in an unconfigured build. + */ +export const DEV: boolean = + typeof __SIBU_DEV__ !== "undefined" + ? !!__SIBU_DEV__ + : typeof (globalThis as any).__SIBU_DEV__ !== "undefined" + ? !!(globalThis as any).__SIBU_DEV__ + : // safe default: off in browser, on in test/dev Node + typeof process !== "undefined" && process.env?.NODE_ENV !== "production"; + +/** + * Returns true when running in development mode, read LIVE at call time. + * + * Unlike {@link DEV} this is a real call, so it observes a `__SIBU_DEV__` + * global that changed after module load. That also means it cannot be folded + * away: use it for behavior (devtools defaults, `strict()`), never as the guard + * on a warning string, or the string ships to production. + * + * @returns `true` in development, `false` in production. */ export function isDev(): boolean { - return typeof (globalThis as any).__SIBU_DEV__ !== "undefined" - ? !!(globalThis as any).__SIBU_DEV__ - : // The bare `__SIBU_DEV__` is a bundler define that only exists in - // production builds; under the test runner it is always undefined, so - // this branch is unreachable here. - /* v8 ignore next 2 */ - typeof __SIBU_DEV__ !== "undefined" - ? __SIBU_DEV__ - : typeof process !== "undefined" && process.env?.NODE_ENV !== "production"; // safe default: off in browser, on in test/dev Node + // globalThis FIRST — the opposite of {@link DEV}, deliberately. + // + // This function exists to be read live, and only the runtime global can + // change after load. Consulting the bare `__SIBU_DEV__` first would let a + // build-time define answer for it in any bundled build, making the runtime + // override dead code there and turning this into a slower copy of `DEV` — + // while the doc above promised a live read. Nothing here needs to fold, so + // ordering costs nothing. + if (typeof (globalThis as any).__SIBU_DEV__ !== "undefined") return !!(globalThis as any).__SIBU_DEV__; + if (typeof __SIBU_DEV__ !== "undefined") return !!__SIBU_DEV__; + return typeof process !== "undefined" && process.env?.NODE_ENV !== "production"; } -// Cache dev mode at module load — avoids 3 typeof checks per call -const _isDev = isDev(); - /** * Assert a condition in dev mode only. No-op in production. + * + * @param condition Asserted expression; a falsy value throws in dev. + * @param message Message for the thrown error, prefixed with `[SibuJS]`. + * @returns Nothing. Throws in dev when `condition` is falsy; in production the + * entire body — message string included — is eliminated at build time. */ export function devAssert(condition: boolean, message: string): void { - if (_isDev && !condition) { + // See `devWarn` for why the define test is repeated inline instead of reusing + // `DEV`. Same reason, same requirement: this body must be able to fold to + // nothing so the message strings never reach a production bundle. + if ((typeof __SIBU_DEV__ !== "undefined" ? __SIBU_DEV__ : DEV) && !condition) { throw new Error(`[SibuJS] ${message}`); } } /** * 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. + * + * @param build Called only in development; returns the warning text. Returning + * an empty string suppresses the warning, which lets a builder do its own + * de-duplication without that bookkeeping escaping into production. + * @returns Nothing. In production the call, the callback, and every string and + * lookup table the callback references are eliminated at build time. + */ +export function devWarnLazy(build: () => string): void { + // Same inline define test as `devWarn`, for the same reason — but this form + // also strips the WORK of composing the message, not just the call. + // + // `if (DEV) { const why = …long text…; devWarn(why); }` does NOT strip in a + // published build: `DEV` arrives as a runtime var there, so the branch is not + // provably dead and its string literals ship. Moving the composition into a + // callback fixes it, because this body folds to nothing, the bundler inlines + // it, and the now-unreferenced closure — with every literal and lookup table + // it touched — is tree-shaken away. + // + // Use this for any diagnostic that branches on the situation, interpolates, + // or reads a table of explanatory text. Use `devWarn` for a plain literal. + if (typeof __SIBU_DEV__ !== "undefined" ? __SIBU_DEV__ : DEV) { + const message = build(); + if (message) console.warn(`[SibuJS] ${message}`); + } +} + export function devWarn(message: string): void { - if (_isDev) { + // The `__SIBU_DEV__` test is repeated INLINE here rather than reusing `DEV`, + // and that redundancy is the point. + // + // `DEV` folds only when it is the first declaration in the emitted module. + // That holds when a consumer bundles this package's SOURCE, but the published + // `dist` chunk carries the bundler's own runtime helpers (`__defProp`, + // `__export`) above it, which is enough to make the downstream inliner give + // up: `DEV` arrives as a runtime `var`, this body never becomes empty, and + // every message string in the library ships to production even though none + // can ever print. + // + // Testing the define directly sidesteps that entirely. With + // `__SIBU_DEV__: false` the condition folds to `false` here regardless of how + // `DEV` was emitted, the body becomes empty, and the bundler then inlines + // this function at each call site and drops the message argument with it — + // so an unguarded `devWarn(…)` costs a production consumer nothing. + // + // With no define, an unqualified `__SIBU_DEV__` resolves to the global of + // that name, so the behaviour is identical to reading `DEV`. + if (typeof __SIBU_DEV__ !== "undefined" ? __SIBU_DEV__ : DEV) { console.warn(`[SibuJS] ${message}`); } } diff --git a/src/core/errors.ts b/src/core/errors.ts index 4d13c0a..f4a8329 100644 --- a/src/core/errors.ts +++ b/src/core/errors.ts @@ -110,6 +110,8 @@ function state(): RuntimeErrorState { * This is application/runtime-global state, NOT request-scoped: under SSR it is * shared by every concurrent request in the process. Install it once at startup * and derive any request-specific detail from the error/context instead. + * + * @returns The previously installed handler, so a caller can restore it. */ export function setRuntimeErrorHandler(handler: RuntimeErrorHandler | null): RuntimeErrorHandler | null { const s = state(); @@ -118,7 +120,11 @@ export function setRuntimeErrorHandler(handler: RuntimeErrorHandler | null): Run return prev; } -/** The currently installed runtime error handler, if any. */ +/** + * Read the currently installed runtime error handler. + * + * @returns The installed handler, or `null` when none is set. + */ export function getRuntimeErrorHandler(): RuntimeErrorHandler | null { return state().handler; } diff --git a/src/core/rendering/context.ts b/src/core/rendering/context.ts index c625b97..4e2708e 100644 --- a/src/core/rendering/context.ts +++ b/src/core/rendering/context.ts @@ -1,9 +1,7 @@ -import { devWarn, isDev } from "../dev"; +import { DEV, devWarn } from "../dev"; import { signal } from "../signals/signal"; import { isSSR } from "../ssr-context"; -const _isDev = isDev(); - /** * Context API for SibuJS — a reactive global value that any component * can read without prop drilling. @@ -72,7 +70,7 @@ export function context(defaultValue: T): Context { // surprise to anyone arriving from React/Vue/Solid. Rather than silently // producing cross-request data bleed, say so at the point of misuse. const warnIfSSR = (method: string): void => { - if (!_isDev || !isSSR()) return; + if (!DEV || !isSSR()) return; devWarn( `context.${method}() called during SSR. SibuJS context() is application-global — it is NOT ` + "isolated per request, so a concurrent request can observe this value. Do not use context() " + @@ -113,7 +111,7 @@ export function context(defaultValue: T): Context { // with a promise-aware restore: because the value is global, that would // still not isolate overlapping async scopes, and would merely make the // hazard harder to see. See docs/architecture/context.md. - if (_isDev && result !== null && typeof (result as { then?: unknown })?.then === "function") { + if (DEV && result !== null && typeof (result as { then?: unknown })?.then === "function") { devWarn( "context.withContext() received an async callback. Scoping is synchronous only — the " + "previous value is restored as soon as the callback returns its promise, so anything " + diff --git a/src/core/rendering/directives.ts b/src/core/rendering/directives.ts index 413ee10..4ce6b10 100644 --- a/src/core/rendering/directives.ts +++ b/src/core/rendering/directives.ts @@ -1,39 +1,65 @@ import { track } from "../../reactivity/track"; +import { devWarnLazy } from "../dev"; import { dispose, registerDisposer } from "./dispose"; +import { captureFocusWithin, restoreFocusWithin } from "./focusPreservation"; import type { NodeChild } from "./types"; /** * Conditional rendering directive. Shows or hides an element reactively. * Unlike `when()`, the element is always created — it just toggles display. * + * Accepts the element either directly or as a thunk, so the same call shape + * works here and in {@link when}. Which one you pass changes nothing: `show` + * never rebuilds, so a thunk is invoked exactly once, immediately. + * * @param condition Reactive getter returning boolean - * @param element The element to show/hide - * @returns The element with reactive display binding + * @param element The element to show/hide, or a function returning it + * @returns The element itself (not a wrapper), with a reactive display binding. + * The caller keeps the live node and can mutate it or attach listeners to it. * * @example * ```ts * const [visible, setVisible] = signal(true); * div([show(() => visible(), span("I toggle!"))]); + * div([show(() => visible(), () => span("also fine"))]); * ``` + * + * TRAP — `show` keeps the element in the DOM and toggles `display`. Use + * {@link when} when the content must not exist at all while hidden (an + * expensive subtree, or one whose mere presence is observable). Note that + * `when` REBUILDS on every condition change, which discards focus and + * selection inside the branch — see the note on `when`. */ -export function show(condition: () => boolean, element: T): T { +export function show(condition: () => boolean, element: T | (() => T)): T { + // Widened from `element: T`. `when` took thunks while `show` took an element, + // so passing one API's shape to the other failed — silently for `when`, and + // for `show` as a `TypeError` about `style` on `undefined` raised from inside + // the directive rather than at the call site. Both shapes work in both places + // now, which removes the mistake rather than reporting it. + const resolved: T = typeof element === "function" ? (element as () => T)() : element; const update = () => { - (element as unknown as HTMLElement).style.display = condition() ? "" : "none"; + (resolved as unknown as HTMLElement).style.display = condition() ? "" : "none"; }; // Register the teardown on the element so disposing the element (e.g. when an // enclosing each/when row is removed) also stops the condition subscription. // Without this the effect — and everything it closes over — leaks forever. - registerDisposer(element, track(update)); - return element; + registerDisposer(resolved, track(update)); + return resolved; } /** * Conditional rendering directive. Renders content only when condition is true. * When false, renders nothing (comment placeholder). Re-evaluates reactively. * - * @param condition Reactive getter returning boolean - * @param thenBranch Function returning element when true - * @param elseBranch Optional function returning element when false + * Each branch may be a THUNK (rebuilt on every condition change) or a bare + * element/value (attached as-is, and re-attached unchanged on every later + * change). Both shapes are accepted so that `when` and {@link show} take the + * same arguments; a bare element used to be accepted and then silently render + * nothing, because the directive called it as a function only. + * + * @param condition Reactive getter; its value is compared to decide the branch + * @param thenBranch Element, value, or function returning one, used when truthy + * @param elseBranch Optional counterpart used when falsy * @returns A Comment anchor that manages the conditional content * * @example @@ -43,6 +69,7 @@ export function show(condition: () => boolean, element: T): T * () => div("Welcome!"), * () => div("Please log in") * ); + * when(() => isLoggedIn(), div("Welcome!")); // also valid * ``` * * GOTCHA — branch factories rebuild only when `condition` changes. A signal @@ -53,13 +80,83 @@ export function show(condition: () => boolean, element: T): T * ``` * Drive per-branch reactivity with a nested getter (or a reactive child), not a * bare read in the factory body. + * + * TRAP — a rebuild DESTROYS the outgoing branch, so focus, selection and any + * in-progress IME composition inside it are lost. The runtime restores focus + * when the rebuilt branch contains an element with the same `id`, `name`, or + * `data-focus-key`, and warns in dev when it cannot. For a subtree holding a + * live edit, prefer {@link match} keyed on a shape key so the subtree is + * rebuilt only when the shape genuinely changes. */ -export function when(condition: () => T, thenBranch: () => NodeChild, elseBranch?: () => NodeChild): Comment { +/** + * Resolve one branch of `when`/`match`, insert it after the anchor, and report + * whether the calling directive OWNS the resulting node. + * + * Ownership decides disposal, and getting it wrong is silent in both + * directions: dispose a node the caller still holds and its reactive bindings + * die with no error; fail to dispose one nobody else references and its + * bindings leak. The rule is that a directive owns only what it produced — + * a node returned by a factory, or a text node built from a primitive. + * + * A factory that closes over ONE element and returns it every time (`() => el`) + * is the element form wearing a function's clothes. It is detected on the + * second attach and demoted to unowned, which stops repeated disposal; the + * first switch-away has already disposed it, so the dev warning names the fix. + */ +function attachBranch( + parent: Node, + anchor: Comment, + branch: NodeChild, + attachedOnce: WeakSet, + where: string, +): { node: Node | null; owned: boolean } { + // A function branch is a FACTORY and is invoked; anything else is already + // the content. `NodeChild` includes `() => NodeChild`, so this one check + // covers thunks, accessors, elements, strings and numbers alike. + const result = typeof branch === "function" ? (branch as () => NodeChild)() : branch; + if (result == null || typeof result === "boolean") return { node: null, owned: false }; + + const node = result instanceof Node ? result : document.createTextNode(String(result)); + const handedIn = node === (branch as unknown as Node); + const seenBefore = attachedOnce.has(node); + const owned = !handedIn && !seenBefore; + + // Recorded unconditionally. Ownership must not depend on the build mode: if + // this bookkeeping were dev-only, a `() => stableEl` factory would be treated + // as unowned in dev and disposed in production — the worst kind of bug, one + // that only exists in the build nobody debugs. + attachedOnce.add(node); + + if (seenBefore) { + // Lazy so the branch-dependent text is composed only in dev; see + // `devWarnLazy` for why an `if (DEV)` block would ship these literals. + devWarnLazy( + () => + `${where}: ${handedIn ? "a branch was given as an element rather than a function" : "a branch factory returned a node it had already returned"}, ` + + "so the SAME node is being re-attached — any state it accumulated while detached (input value, scroll " + + "position, classes set imperatively) comes back with it. Its reactive bindings are left intact rather than " + + "disposed, because the node is not this directive's to tear down. Return a FRESH node per call " + + "(`() => div(…)`) for rebuild semantics, or keep this form deliberately when the reuse is what you want.", + ); + } + + parent.insertBefore(node, anchor.nextSibling); + return { node, owned }; +} + +export function when(condition: () => T, thenBranch: NodeChild, elseBranch?: NodeChild): Comment { const anchor = document.createComment("when"); let currentNode: Node | null = null; let lastCondition: T | undefined; let initialized = false; + // Tracks whether a bare-element branch has already been attached once, so the + // reuse warning fires on genuine reuse rather than on first render. + const attachedOnce = new WeakSet(); + // Whether `currentNode` was produced BY this directive (a factory call, or a + // text node built from a primitive) or merely handed to it. Only the former + // may be disposed — see the teardown below. + let currentNodeOwned = false; const update = () => { // Always evaluate condition to register reactive dependencies @@ -72,19 +169,32 @@ export function when(condition: () => T, thenBranch: () => NodeChild, elseBra if (initialized && show === lastCondition) return; lastCondition = show; - // Remove previous node + // Snapshot focus before the outgoing branch is detached — afterwards + // `document.activeElement` has already fallen back to . + const focused = currentNode ? captureFocusWithin([currentNode]) : null; + + // Remove previous node. + // + // Disposing is ownership-dependent. A FACTORY branch is rebuilt on the next + // switch, so its old node is garbage and must be disposed or its bindings + // leak. A BARE ELEMENT branch is the same node every time: the caller built + // it, holds a reference to it, and gets it back on the next switch. + // Disposing that node tore down every reactive binding on it, so the + // element came back inert — its reactive class/style/text silently stopped + // updating, with nothing logged. Detaching is enough; teardown belongs to + // whoever created it. if (currentNode?.parentNode) { - dispose(currentNode); + if (currentNodeOwned) dispose(currentNode); currentNode.parentNode.removeChild(currentNode); currentNode = null; } - const result = show ? thenBranch() : elseBranch ? elseBranch() : null; - if (result != null) { - const node = result instanceof Node ? result : document.createTextNode(String(result)); - parent.insertBefore(node, anchor.nextSibling); - currentNode = node; - } + const branch = show ? thenBranch : elseBranch !== undefined ? elseBranch : null; + const attached = attachBranch(parent, anchor, branch, attachedOnce, "when"); + currentNode = attached.node; + currentNodeOwned = attached.owned; + + restoreFocusWithin(focused, currentNode ? [currentNode] : [], "when"); initialized = true; }; @@ -127,17 +237,37 @@ export function when(condition: () => T, thenBranch: () => NodeChild, elseBra * GOTCHA — like `when()`, a case factory rebuilds only when the matched key * changes. A signal read eagerly inside a case is frozen at build time; use a * nested getter (`() => div(() => label())`) for reactive per-case content. + * + * THIS IS THE KEYING PATTERN for subtrees that hold a live edit. Because the + * subtree is rebuilt only when the KEY changes — not on every read of every + * signal inside it — a form can update its contents through reactive attributes + * and text children while its inputs keep focus, selection and IME state: + * + * ```ts + * // Rebuilds only when the form's shape changes, not on every keystroke. + * match( + * () => `${entity()}:${mode()}`, + * { "user:edit": () => UserForm(), "user:view": () => UserCard() }, + * ); + * ``` + * + * Cases and the fallback accept a bare element as well as a factory, matching + * {@link when} and {@link show}. A bare element is re-attached rather than + * rebuilt, so it keeps whatever state it accumulated. */ export function match( value: () => T, - cases: Record NodeChild>, - fallback?: () => NodeChild, + cases: Record, + fallback?: NodeChild, ): Comment { const anchor = document.createComment("match"); let currentNode: Node | null = null; let lastKey: string | undefined; let initialized = false; + const attachedOnce = new WeakSet(); + // See `when`: only a node this directive produced may be disposed. + let currentNodeOwned = false; const update = () => { // Always evaluate value() to register reactive dependencies @@ -150,21 +280,25 @@ export function match( if (initialized && key === lastKey) return; lastKey = key; + // Same as `when`: the caret must be snapshotted while the outgoing case is + // still attached. `match` is the pattern recommended for AVOIDING rebuilds, + // but a genuine key change still replaces the subtree. + const focused = currentNode ? captureFocusWithin([currentNode]) : null; + if (currentNode?.parentNode) { - dispose(currentNode); + if (currentNodeOwned) dispose(currentNode); currentNode.parentNode.removeChild(currentNode); currentNode = null; } - const renderFn = cases[key] || fallback; - if (renderFn) { - const result = renderFn(); - if (result != null) { - const node = result instanceof Node ? result : document.createTextNode(String(result)); - parent.insertBefore(node, anchor.nextSibling); - currentNode = node; - } - } + // `Object.hasOwn` rather than `||` so a case whose value is legitimately + // falsy (an empty string, 0) still wins over the fallback. + const branch = Object.hasOwn(cases, key) ? cases[key] : fallback; + const attached = attachBranch(parent, anchor, branch ?? null, attachedOnce, "match"); + currentNode = attached.node; + currentNodeOwned = attached.owned; + + restoreFocusWithin(focused, currentNode ? [currentNode] : [], "match"); initialized = true; }; diff --git a/src/core/rendering/dispose.ts b/src/core/rendering/dispose.ts index 2ff8780..22ce05c 100644 --- a/src/core/rendering/dispose.ts +++ b/src/core/rendering/dispose.ts @@ -1,4 +1,4 @@ -import { devWarn, isDev } from "../dev"; +import { DEV, devWarn } from "../dev"; import { reportError } from "../errors"; const elementDisposers = new WeakMap void>>(); @@ -41,8 +41,6 @@ export function reportDrainRunaway(label: string, executed: number, remaining: n } // Dev-mode only: track active bindings to detect orphans. -// In production, _isDev is false and the counter is never touched. -const _isDev = isDev(); let activeBindingCount = 0; /** @@ -56,7 +54,7 @@ export function registerDisposer(node: Node, teardown: () => void): void { elementDisposers.set(node, disposers); } disposers.push(teardown); - if (_isDev) activeBindingCount++; + if (DEV) activeBindingCount++; } /** @@ -75,7 +73,7 @@ export function unregisterDisposer(node: Node, teardown: () => void): void { const index = disposers.indexOf(teardown); if (index === -1) return; disposers.splice(index, 1); - if (_isDev) activeBindingCount--; + if (DEV) activeBindingCount--; if (disposers.length === 0) elementDisposers.delete(node); } @@ -128,7 +126,7 @@ export function dispose(node: Node): void { // re-run these or land in an infinite cycle. const snapshot = pending.slice(); elementDisposers.delete(current); - if (_isDev) activeBindingCount -= snapshot.length; + if (DEV) activeBindingCount -= snapshot.length; for (let i = 0; i < snapshot.length; i++) { if (executed >= MAX_DRAIN_TEARDOWNS) { @@ -140,7 +138,7 @@ export function dispose(node: Node): void { const rest = snapshot.slice(i); const added = elementDisposers.get(current); elementDisposers.set(current, added ? rest.concat(added) : rest); - if (_isDev) activeBindingCount += rest.length; + if (DEV) activeBindingCount += rest.length; reportDrainRunaway("dispose", executed, rest.length + (added?.length ?? 0)); runaway = true; break; @@ -202,10 +200,12 @@ export function replaceChildrenSafely(parent: ParentNode, ...next: Node[]): void /** * Check for potential binding leaks. Returns the number of active DOM bindings. * In dev mode, logs a warning if the count exceeds the threshold. - * In production, _isDev is false so the counter is always 0. + * In production, DEV is false so the counter is always 0. + * + * @returns Diagnostic counts of nodes still holding registered disposers. */ export function checkLeaks(warnThreshold = 0): number { - if (!_isDev) return 0; + if (!DEV) return 0; if (warnThreshold > 0 && activeBindingCount > warnThreshold) { devWarn( `checkLeaks: ${activeBindingCount} active DOM bindings detected. ` + diff --git a/src/core/rendering/each.ts b/src/core/rendering/each.ts index d5c0006..7757e32 100644 --- a/src/core/rendering/each.ts +++ b/src/core/rendering/each.ts @@ -1,13 +1,11 @@ import { batch } from "../../reactivity/batch"; import { track } from "../../reactivity/track"; -import { devAssert, devWarn, isDev } from "../dev"; +import { DEV, devAssert, devWarn } from "../dev"; import { reportError } from "../errors"; import { signal } from "../signals/signal"; import { dispose, registerDisposer } from "./dispose"; import type { NodeChild } from "./types"; -const _isDev = isDev(); - /** * Resolves a NodeChild to a real Node. * - If it's a function, calls recursively. @@ -215,7 +213,7 @@ export function each( // Duplicate keys collapse to a single node reference, so two array // positions would share one DOM node — one row silently vanishes and // order can drift. Warn loudly in dev (mirrors bindChildNode). - if (_isDev && keyIndexMap.has(newKeys[i])) { + if (DEV && keyIndexMap.has(newKeys[i])) { devWarn( `each: duplicate key "${String(newKeys[i])}" at index ${i} (first seen at ${keyIndexMap.get(newKeys[i])}). ` + "Keys must be unique — duplicates cause rows to be dropped or mis-ordered.", diff --git a/src/core/rendering/focusPreservation.ts b/src/core/rendering/focusPreservation.ts new file mode 100644 index 0000000..e164b0e --- /dev/null +++ b/src/core/rendering/focusPreservation.ts @@ -0,0 +1,238 @@ +import { devWarnLazy } from "../dev"; + +// --------------------------------------------------------------------------- +// Keeping the caret alive across a reactive rebuild. +// +// A reactive block that re-creates its children destroys the focused node. The +// browser then moves focus to , and the user's next keystroke goes +// nowhere — one character typed into an input inside such a block ends the +// edit. Nothing throws and nothing is logged, so it reads as the application +// losing what you typed rather than as a framework behaviour. +// +// This module does the honest half of the fix: where the rebuilt subtree +// contains an element whose identity can be RE-ESTABLISHED, focus and the +// selection range come back; where it cannot, dev says so and names the way +// out. It deliberately does not guess. Focusing the wrong element is worse than +// focusing nothing, because the next keystroke then lands in a field the user +// was not editing. +// +// What identity means here, in priority order: +// 1. `data-focus-key` — an explicit author-supplied identity. +// 2. `id` +// 3. `name` +// All three are stable across a rebuild by construction, because the author +// writes them into the factory that rebuilds. Structural matching ("the second +// input in the third div") is deliberately NOT used: it silently re-targets the +// caret whenever the shape changes, which is exactly when a rebuild happens. +// +// A match must also be UNIQUE in the rebuilt subtree. `name` is shared by +// design — every radio in a group carries the same one — so taking the first +// hit would put the caret on a sibling control the user was not using. An +// ambiguous match is treated as no match, and warned about. +// +// Focus is restored only when it was LOST (it fell to ). If something in +// the rebuild moved focus deliberately, that wins: an autofocusing branch is +// not something to fight. +// +// NOT preserved, and it cannot be: an in-progress IME composition. The +// composition is owned by the DOM node, so destroying the node ends it. A +// keyed subtree that never rebuilds mid-edit is the only real fix, which is +// what the warning points at. +// --------------------------------------------------------------------------- + +/** Identity attributes consulted, in priority order. */ +const IDENTITY_ATTRS = ["data-focus-key", "id", "name"] as const; + +/** + * Input types whose selection range is readable/writable. Reading + * `selectionStart` on any other type throws in some engines, so the set is an + * allowlist rather than a try/catch. + */ +const SELECTABLE_TYPES = new Set(["text", "search", "url", "tel", "password", ""]); + +export interface FocusSnapshot { + /** The element that had focus, kept so we can tell whether it survived. */ + el: Element; + /** `attr=value` identity pair, or null when the element has no stable identity. */ + attr: string | null; + value: string | null; + tag: string; + start: number | null; + end: number | null; + direction: string | null; +} + +function selectionOf(el: Element): Pick { + const tag = el.tagName; + const selectable = + tag === "TEXTAREA" || (tag === "INPUT" && SELECTABLE_TYPES.has((el as HTMLInputElement).type.toLowerCase())); + if (!selectable) return { start: null, end: null, direction: null }; + const field = el as HTMLInputElement | HTMLTextAreaElement; + return { start: field.selectionStart, end: field.selectionEnd, direction: field.selectionDirection }; +} + +/** + * Snapshot the focused element if it lives inside `nodes`, which are about to + * be removed. Returns null — the overwhelmingly common case — when nothing is + * focused or focus is elsewhere on the page. + * + * @param nodes The nodes a rebuild is about to discard. + * @returns A snapshot to pass to {@link restoreFocusWithin}, or null when there + * is no focus at stake. + */ +export function captureFocusWithin(nodes: readonly Node[]): FocusSnapshot | null { + if (nodes.length === 0 || typeof document === "undefined") return null; + const active = document.activeElement; + // `` is the "nothing is focused" resting state. Bailing here keeps this + // to one property read on every rebuild in a page where no field is active. + if (!active || active === document.body) return null; + + let inside = false; + for (let i = 0; i < nodes.length; i++) { + const n = nodes[i]; + if (n === active || (n.nodeType === 1 && (n as Element).contains(active))) { + inside = true; + break; + } + } + if (!inside) return null; + + let attr: string | null = null; + let value: string | null = null; + for (let i = 0; i < IDENTITY_ATTRS.length; i++) { + const candidate = active.getAttribute(IDENTITY_ATTRS[i]); + if (candidate) { + attr = IDENTITY_ATTRS[i]; + value = candidate; + break; + } + } + + return { el: active, attr, value, tag: active.tagName, ...selectionOf(active) }; +} + +/** Quote a value for use inside an attribute selector. */ +function quote(value: string): string { + return `"${value.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; +} + +/** + * Find the ONE element matching `selector`, or nothing. + * + * Ambiguity is treated as failure, not as a reason to pick the first hit. + * `name` in particular is shared by design — every radio in a group carries the + * same one — so first-match would restore focus onto a sibling control and the + * user's next keystroke would act on something they were not using. That is + * strictly worse than losing focus, which is at least visible. When the match + * is ambiguous the caller warns instead. + */ +function findUniqueByIdentity(nodes: readonly Node[], selector: string): { el: Element | null; count: number } { + let el: Element | null = null; + let count = 0; + for (let i = 0; i < nodes.length; i++) { + const n = nodes[i]; + if (n.nodeType !== 1) continue; + const root = n as Element; + if (root.matches(selector)) { + count++; + el ??= root; + } + const inner = root.querySelectorAll(selector); + for (let j = 0; j < inner.length; j++) { + count++; + el ??= inner[j]; + } + // Two is already ambiguous; counting the rest tells us nothing more. + if (count > 1) return { el: null, count }; + } + return count === 1 ? { el, count } : { el: null, count }; +} + +/** Give `el` focus and put the snapshotted selection range back. */ +function refocus(el: Element, snapshot: FocusSnapshot): void { + (el as HTMLElement).focus(); + if (snapshot.start !== null && typeof (el as HTMLInputElement).setSelectionRange === "function") { + const field = el as HTMLInputElement; + // The rebuilt value may be shorter than the old one; clamping keeps + // setSelectionRange from throwing on an out-of-range index. + const max = field.value.length; + const start = Math.min(snapshot.start, max); + const end = Math.min(snapshot.end ?? start, max); + field.setSelectionRange(start, end, (snapshot.direction as "forward" | "backward" | "none") ?? "none"); + } +} + +/** + * Put focus (and the selection range) back after a rebuild, or explain in dev + * why it could not be done. + * + * @param snapshot Result of {@link captureFocusWithin}, or null. + * @param nodes The nodes the rebuild produced. + * @param where Name of the calling directive, used in the dev warning. + * @returns Nothing. + */ +export function restoreFocusWithin(snapshot: FocusSnapshot | null, nodes: readonly Node[], where: string): void { + if (!snapshot) return; + if (typeof document === "undefined") return; + + const active = document.activeElement; + + // Still focused — nothing happened worth undoing. Re-focusing here would only + // risk an unwanted scroll. + if (active === snapshot.el) return; + + // Focus is on some OTHER real element. Something during the rebuild moved it + // deliberately (a branch that autofocuses its first field, say), and stealing + // it back would fight the application. Only a focus that fell to the document + // is a focus that was LOST. + // + // Testing where focus actually IS — rather than whether the old node is still + // connected — is what catches the case where the node survives the rebuild + // but is blurred by being MOVED: re-inserting a focused element blurs it in + // real browsers, so `isConnected` reported "nothing to do" while the caret + // was already gone. + if (active && active !== document.body && active !== document.documentElement) return; + + // The node itself survived (reused or reordered) and merely lost focus. No + // identity matching needed — this IS the element the user was editing. + if (snapshot.el.isConnected) { + refocus(snapshot.el, snapshot); + return; + } + + let ambiguous = false; + if (snapshot.attr && snapshot.value) { + const { el: replacement, count } = findUniqueByIdentity(nodes, `[${snapshot.attr}=${quote(snapshot.value)}]`); + // Same identity AND same element type. A `data-focus-key` reused across a + // and a