diff --git a/.changeset/owner-detached-dispose-4444.md b/.changeset/owner-detached-dispose-4444.md new file mode 100644 index 0000000000..f978c952c8 --- /dev/null +++ b/.changeset/owner-detached-dispose-4444.md @@ -0,0 +1,7 @@ +--- +"@solidjs/signals": patch +"solid-js": patch +"@solidjs/web": patch +--- + +`createOwner()`'s `dispose` can be passed around as a plain function — `queueMicrotask(o.dispose)`, `setTimeout(o.dispose)` — instead of throwing `TypeError: Cannot read properties of undefined (reading '_flags')` (#4444). The method was a shared function reading its owner from `this`, which a detached call leaves undefined; the exported `createOwner` now binds it, as `createRoot`'s dispose already was. Internal owners (`mapArray` rows, boundaries) keep the shared method, so they pay no allocation. `NoHydration`, `Portal` and a frame's fill use the unbound owner too, through `solid-js/internal`, so apps that never call `createOwner` pay nothing. diff --git a/packages/signals/src/core/index.ts b/packages/signals/src/core/index.ts index 7c0e40fd4c..28a1d16481 100644 --- a/packages/signals/src/core/index.ts +++ b/packages/signals/src/core/index.ts @@ -27,6 +27,7 @@ export { } from "./external.js"; export { createOwner, + createPublicOwner, createRoot, dispose, getNextChildId, diff --git a/packages/signals/src/core/owner.ts b/packages/signals/src/core/owner.ts index 626cda0f10..db370d5d2a 100644 --- a/packages/signals/src/core/owner.ts +++ b/packages/signals/src/core/owner.ts @@ -444,6 +444,25 @@ export function createOwner(options?: { id?: string; transparent?: boolean }) { return owner; } +// The exported `createOwner` (the internal one is above): its `dispose` is +// bound to the owner, so it can be handed around as a plain function, as +// `createRoot`'s dispose can (#4444). Internal owners (a `mapArray` row, a +// boundary) keep the shared `this`-reading method: they are only ever disposed +// as `o.dispose()`, and a bound function per row would be an allocation per row. +/** + * Creates a fresh owner attached as a child of the current owner (or as a + * detached root if there is none), to group cleanups and context under one + * scope. Its `dispose()` tears that scope down and is bound to the owner, so it + * can be passed around as a plain function: `queueMicrotask(owner.dispose)`. + * To host a reactive scope outside a component, prefer `createRoot()`; to + * re-enter a captured owner, use `runWithOwner()`. + */ +export function createPublicOwner(options?: { id?: string; transparent?: boolean }) { + const owner = createOwner(options); + owner.dispose = disposeRootSelf.bind(owner); + return owner; +} + /** * Creates a reactive root — an owner scope with its own `dispose()`. A root * created inside an existing owner is owned by it and is disposed when the diff --git a/packages/signals/src/index.ts b/packages/signals/src/index.ts index c91b6cef24..9d0144dc0c 100644 --- a/packages/signals/src/index.ts +++ b/packages/signals/src/index.ts @@ -6,7 +6,7 @@ export { NotReadyError, TimeoutError, createContext, - createOwner, + createPublicOwner as createOwner, createRoot, runWithOwner, flush, @@ -38,6 +38,15 @@ export { * outside dev builds. */ export { setConsoleFooter } from "./core/index.js"; +/** + * @internal `createOwner` as the runtimes in this repo use it (`NoHydration`, + * `Portal`, a frame's fill): its `dispose` is the shared `this`-reading + * method, not the bound one the exported `createOwner` gives (#4444). They + * dispose it as `o.dispose()` or with its parent, so apps that never call + * `createOwner` pay no bind. Client runtime only: the server has its own + * owners. + */ +export { createOwner as createScopeOwner } from "./core/index.js"; /** @internal The flush round counter (scheduler.ts): hydration's served pass tells its own round from a later one. */ export { clock } from "./core/index.js"; /** @internal Dev duplicate-copy check: solid-js marks components and context reads. */ diff --git a/packages/signals/tests/owner-detached-dispose-4444.test.ts b/packages/signals/tests/owner-detached-dispose-4444.test.ts new file mode 100644 index 0000000000..2f6fe6c8c6 --- /dev/null +++ b/packages/signals/tests/owner-detached-dispose-4444.test.ts @@ -0,0 +1,126 @@ +/** + * #4444: `createOwner()`'s `dispose` is handed around as a plain function + * (`queueMicrotask(o.dispose)`, `setTimeout(o.dispose)`, an event listener) + * and threw `TypeError: Cannot read properties of undefined (reading + * '_flags')`: the method was a shared function that read its owner from + * `this`, which a detached call leaves undefined. Nothing about the effect + * in the report mattered — a bare owner threw the same way. + * + * No F-/D- rule governs this: it is the call shape of the owner's disposer, + * not a reactive semantic. The disposal it performs is unchanged: everything + * under the owner dies, cleanups run, and an effect whose first run is still + * scheduled never runs. + */ +import { describe, expect, it } from "vitest"; +import { + createEffect, + createOwner, + createRenderEffect, + flush, + isDisposed, + onCleanup, + runWithOwner +} from "../src/index.js"; + +const macrotask = () => new Promise(r => setTimeout(r, 0)); + +describe("createOwner dispose called detached (#4444)", () => { + it("the issue's shape: a scheduled render effect, owner disposed from a microtask", async () => { + const ran: string[] = []; + const o = createOwner(); + runWithOwner(o, () => { + createRenderEffect( + () => {}, + () => { + ran.push("effect"); + return () => ran.push("cleanup"); + }, + { schedule: true } + ); + queueMicrotask(o.dispose); + }); + // The flush microtask was queued at creation, ahead of the dispose, so the + // first run lands and the dispose then tears it down. + await macrotask(); + expect(isDisposed(o)).toBe(true); + expect(ran).toEqual(["effect", "cleanup"]); + }); + + it("a detached dispose disposes, with `self` defaulting to true", () => { + const o = createOwner(); + let cleaned = 0; + runWithOwner(o, () => onCleanup(() => cleaned++)); + const dispose = o.dispose; + dispose(); + expect(isDisposed(o)).toBe(true); + expect(cleaned).toBe(1); + }); + + it("a detached dispose(false) keeps the owner and disposes its contents", () => { + const o = createOwner(); + let cleaned = 0; + runWithOwner(o, () => onCleanup(() => cleaned++)); + const dispose = o.dispose; + dispose(false); + expect(isDisposed(o)).toBe(false); + expect(cleaned).toBe(1); + }); + + for (const kind of ["createRenderEffect", "createEffect"] as const) { + for (const schedule of [true, false]) { + const make = kind === "createRenderEffect" ? createRenderEffect : createEffect; + const label = `${kind}${schedule ? " { schedule: true }" : ""}`; + + it(`${label}: detached dispose before the first scheduled run — effects never run`, async () => { + const ran: string[] = []; + const o = createOwner(); + runWithOwner(o, () => { + for (let i = 0; i < 3; i++) + make( + () => i, + v => { + ran.push(`e${v}`); + }, + schedule ? { schedule: true } : undefined + ); + }); + const dispose = o.dispose; + dispose(); + flush(); + await macrotask(); + expect(isDisposed(o)).toBe(true); + // A render effect without `schedule` runs its effect at creation; the + // rest defer to the flush, which the disposal empties first. + expect(ran).toEqual(kind === "createRenderEffect" && !schedule ? ["e0", "e1", "e2"] : []); + }); + + for (const when of ["microtask", "macrotask"] as const) { + it(`${label}: detached dispose from a ${when}`, async () => { + const ran: string[] = []; + const o = createOwner(); + runWithOwner(o, () => { + for (let i = 0; i < 3; i++) + make( + () => i, + v => { + ran.push(`e${v}`); + return () => ran.push(`c${v}`); + }, + schedule ? { schedule: true } : undefined + ); + }); + if (when === "microtask") queueMicrotask(o.dispose); + else setTimeout(o.dispose, 0); + await macrotask(); + await macrotask(); + flush(); + expect(isDisposed(o)).toBe(true); + // Each effect ran once (the creation flush precedes the dispose) and + // was cleaned up by the dispose; none runs again after it. + expect(ran.filter(s => s[0] === "e").sort()).toEqual(["e0", "e1", "e2"]); + expect(ran.filter(s => s[0] === "c").sort()).toEqual(["c0", "c1", "c2"]); + }); + } + } + } +}); diff --git a/packages/solid/src/client/hydration.ts b/packages/solid/src/client/hydration.ts index adea48aa36..86a9cb1aa7 100644 --- a/packages/solid/src/client/hydration.ts +++ b/packages/solid/src/client/hydration.ts @@ -39,7 +39,7 @@ import { type StoreOptions, type StoreSetter, type RevealOrder, - createOwner, + createScopeOwner, createRoot as coreRoot, getContext, setContext, @@ -3120,7 +3120,7 @@ function hydratedCreateLoadingBoundary( * ``` */ export function NoHydration(props: { children: SolidElement }): SolidElement { - const o = createOwner(); + const o = createScopeOwner(); return runWithOwner(o, () => { setContext(NoHydrateContext, true); // Claiming, not the bare flag: a rejected fragment's fresh render diff --git a/packages/solid/src/internal.ts b/packages/solid/src/internal.ts index 9a14d48a5f..b49b8cdfcf 100644 --- a/packages/solid/src/internal.ts +++ b/packages/solid/src/internal.ts @@ -27,6 +27,11 @@ * build boundaries without the components, and so are `sharedConfig` * (the hydration/SSR coordination object) and the `$DEVCOMP` brand. * + * Also re-exported from `@solidjs/signals`: `createScopeOwner`, the owner the + * runtimes create for their own scopes (`NoHydration`, `Portal`, a frame's + * fill). It keeps the shared `this`-reading `dispose`, where the public + * `createOwner` binds it per owner (#4444). + * * Not here: the container-trace materializer. It lives in its own entry, * `solid-js/internal/container-trace`, so the store engine it builds on can * be a lazy chunk — a re-export from this (eagerly imported) module would @@ -61,7 +66,8 @@ export { SOURCE_PROXY, SOURCE_MEMO, SOURCE_MERGE, - type SourceKind + type SourceKind, + createScopeOwner } from "@solidjs/signals"; /** diff --git a/packages/web/frames/src/client.ts b/packages/web/frames/src/client.ts index 3719206b29..3b210660bb 100644 --- a/packages/web/frames/src/client.ts +++ b/packages/web/frames/src/client.ts @@ -19,7 +19,6 @@ import { OBSERVE, createMemo, - createOwner, createRenderEffect, createSignal, getOwner, @@ -71,7 +70,8 @@ import { setWriteHook, signalName, sharedConfig, - storeName + storeName, + createScopeOwner } from "solid-js/internal"; // The container tier (DR-2 case 3): server projections cross the border as @@ -766,8 +766,8 @@ function slotsFor(props: Record, scope?: ClaimScope) { // hydrate pass consumes no id from the adopting component's counter // (a keyed sibling after the frame keys the same whether a fill // mounted at t=0 or after a hold; the claim window has its own id). - const fillOwner = streamInvoke ? createOwner() : null; - const owner = fillOwner || createOwner({ transparent: true }); + const fillOwner = streamInvoke ? createScopeOwner() : null; + const owner = fillOwner || createScopeOwner({ transparent: true }); fillScopes.set(key, owner); ctx.onCleanup(() => { if (fillScopes.get(key) === owner) fillScopes.delete(key); @@ -825,7 +825,7 @@ function slotsFor(props: Record, scope?: ClaimScope) { // re-call with existing nodes must render for real — claiming would // no-op its inserts and silently drop whatever the re-call // displaced, e.g. moved-out {$frame} region ranges (#547). - const claim = adopted && runWithOwner(owner, () => createOwner({ id: prefix })); + const claim = adopted && runWithOwner(owner, () => createScopeOwner({ id: prefix })); const value = adopted ? claimRender(prefix, claim, ctx.existing, evaluate, scope) : fillOwner diff --git a/packages/web/src/index.ts b/packages/web/src/index.ts index 06c4f93ebe..412f2a2fb6 100644 --- a/packages/web/src/index.ts +++ b/packages/web/src/index.ts @@ -13,7 +13,6 @@ import { import { createComponent, createMemo, - createOwner, createRoot, createSignal, getOwner, @@ -28,7 +27,7 @@ import { type Setter, type DynamicOptions } from "solid-js"; -import { dynamicCore, sharedConfig } from "solid-js/internal"; +import { createScopeOwner, dynamicCore, sharedConfig } from "solid-js/internal"; import type { JSX } from "../jsx/jsx.js"; export * from "./client.js"; @@ -135,7 +134,7 @@ export function Portal(props: { mount?: Element; children: JSX.Element }): JSX.E // child-id counter and shift every hydration id after the portal. Both // sides consume exactly one slot from the parent instead — the owner // here, `getNextChildId` in the server Portal. - return runWithOwner(createOwner(), () => portalImpl(props)) as unknown as JSX.Element; + return runWithOwner(createScopeOwner(), () => portalImpl(props)) as unknown as JSX.Element; } function portalImpl(props: { mount?: Element; children: JSX.Element }): JSX.Element {