From 49bdc24ef5f1a2862928e57ca0acb84b0551372b Mon Sep 17 00:00:00 2001 From: ATOM00blue <219721791+ATOM00blue@users.noreply.github.com> Date: Thu, 21 May 2026 08:03:04 +0530 Subject: [PATCH 1/2] fix: keep onCleanup order consistent between dev and prod In dev builds, components are wrapped in an extra owner scope by `devComponent` so devtools can observe component boundaries. This wrapper does not exist in production, which caused `onCleanup` callbacks registered directly in a component body to fire in a different order on disposal than in production. Register such cleanups on the same owner they would be in production by skipping past any dev component wrapper(s). The component owner returned by `getOwner()` is left unchanged so devtools still works. Closes #1561 --- packages/solid/src/reactive/signal.ts | 13 ++- packages/solid/test/dev.spec.ts | 126 +++++++++++++++++++++++++- 2 files changed, 135 insertions(+), 4 deletions(-) diff --git a/packages/solid/src/reactive/signal.ts b/packages/solid/src/reactive/signal.ts index d47844c01..9c45277e9 100644 --- a/packages/solid/src/reactive/signal.ts +++ b/packages/solid/src/reactive/signal.ts @@ -1005,10 +1005,17 @@ export function onMount(fn: () => void) { * @description https://docs.solidjs.com/reference/lifecycle/on-cleanup */ export function onCleanup any>(fn: T): T { - if (Owner === null) + let o = Owner; + // In dev, components are wrapped in an extra owner scope so that devtools can + // observe them (see `devComponent`). That wrapper does not exist in production, + // which means cleanups registered directly in a component body would otherwise + // be ordered differently on disposal. Skip past any dev component wrapper(s) so + // the cleanup is registered on the same owner it would be in production. See #1561. + if (IS_DEV) while (o && (o as DevComponent).component) o = o.owner; + if (o === null) IS_DEV && console.warn("cleanups created outside a `createRoot` or `render` will never be run"); - else if (Owner.cleanups === null) Owner.cleanups = [fn]; - else Owner.cleanups.push(fn); + else if (o.cleanups === null) o.cleanups = [fn]; + else o.cleanups.push(fn); return fn; } diff --git a/packages/solid/test/dev.spec.ts b/packages/solid/test/dev.spec.ts index 4133ec4d6..4f5df9f0c 100644 --- a/packages/solid/test/dev.spec.ts +++ b/packages/solid/test/dev.spec.ts @@ -7,7 +7,9 @@ import { createComputed, DEV, createContext, - createComponent + createComponent, + onCleanup, + untrack } from "../src/index.js"; import type { DevComponent } from "../src/reactive/signal.js"; import { createStore, unwrap, DEV as STORE_DEV } from "../store/src/index.js"; @@ -183,4 +185,126 @@ describe("Dev features", () => { }, props); }); }); + + // https://github.com/solidjs/solid/issues/1561 + // The dev-only component wrapper added by `createComponent`/`devComponent` + // must not change the order in which `onCleanup` callbacks fire on disposal + // compared to production (where no wrapper exists). + describe("onCleanup order is consistent with production (#1561)", () => { + test("cleanups in a component body interleave with sibling cleanups", () => { + const devOrder: string[] = []; + createRoot(dispose => { + onCleanup(() => devOrder.push("before")); + createComponent(function Child() { + onCleanup(() => devOrder.push("child")); + return null; + }, {}); + onCleanup(() => devOrder.push("after")); + dispose(); + }); + + // Simulate the production code path (no dev wrapper) by calling the + // component directly under `untrack`, like `createComponent` does in prod. + const prodOrder: string[] = []; + createRoot(dispose => { + onCleanup(() => prodOrder.push("before")); + untrack(() => + (function Child() { + onCleanup(() => prodOrder.push("child")); + return null; + })() + ); + onCleanup(() => prodOrder.push("after")); + dispose(); + }); + + expect(devOrder).toEqual(prodOrder); + expect(devOrder).toEqual(["after", "child", "before"]); + }); + + test("nested component cleanups interleave with sibling cleanups", () => { + const devOrder: string[] = []; + createRoot(dispose => { + onCleanup(() => devOrder.push("root-before")); + createComponent(function Outer() { + onCleanup(() => devOrder.push("outer-before")); + createComponent(function Inner() { + onCleanup(() => devOrder.push("inner")); + return null; + }, {}); + onCleanup(() => devOrder.push("outer-after")); + return null; + }, {}); + onCleanup(() => devOrder.push("root-after")); + dispose(); + }); + + const prodOrder: string[] = []; + createRoot(dispose => { + onCleanup(() => prodOrder.push("root-before")); + untrack(() => + (function Outer() { + onCleanup(() => prodOrder.push("outer-before")); + untrack(() => + (function Inner() { + onCleanup(() => prodOrder.push("inner")); + return null; + })() + ); + onCleanup(() => prodOrder.push("outer-after")); + return null; + })() + ); + onCleanup(() => prodOrder.push("root-after")); + dispose(); + }); + + expect(devOrder).toEqual(prodOrder); + expect(devOrder).toEqual([ + "root-after", + "outer-after", + "inner", + "outer-before", + "root-before" + ]); + }); + + test("component cleanups still run when the component is re-created", () => { + const log: string[] = []; + let setKey!: (v: number) => void; + const dispose = createRoot(dispose => { + const [key, _setKey] = createSignal(0); + setKey = _setKey; + createEffect(() => { + const k = key(); + createComponent(function Child() { + onCleanup(() => log.push(`cleanup-${k}`)); + return null; + }, {}); + }); + return dispose; + }); + // Re-running the effect must dispose the previous component instance and + // fire its cleanup, even though the cleanup lives on the (persistent) parent. + setKey(1); + setKey(2); + dispose(); + expect(log).toEqual(["cleanup-0", "cleanup-1", "cleanup-2"]); + }); + + test("getOwner inside a component still returns the dev component wrapper", () => { + createRoot(dispose => { + const root = getOwner(); + createComponent(function MyComponent() { + const owner = getOwner() as DevComponent<{}>; + // owner identity is the dev wrapper, not the parent (preserves devtools) + expect(owner).not.toBe(root); + expect(owner.component).toBe(MyComponent); + expect(owner.owner).toBe(root); + return null; + }, {}); + dispose(); + }); + }); + }); }); From a4dba97e5ae6ad8d4f8bad26ff3dfc082b8d8efa Mon Sep 17 00:00:00 2001 From: ATOM00blue <219721791+ATOM00blue@users.noreply.github.com> Date: Thu, 21 May 2026 08:55:32 +0530 Subject: [PATCH 2/2] chore: add changeset for onCleanup dev/prod consistency fix --- .changeset/dev-owner-cleanup-order.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/dev-owner-cleanup-order.md diff --git a/.changeset/dev-owner-cleanup-order.md b/.changeset/dev-owner-cleanup-order.md new file mode 100644 index 000000000..381657f07 --- /dev/null +++ b/.changeset/dev-owner-cleanup-order.md @@ -0,0 +1,5 @@ +--- +"solid-js": patch +--- + +Keep `onCleanup` ordering consistent between development and production builds. In dev, components are wrapped in an extra owner scope for devtools; cleanups registered directly in a component body now register on the same owner they would have in production, so disposal order matches across build modes.