fix: keep onCleanup order consistent between dev and prod

.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.

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<T extends () => 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<any>).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;
 }
 

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();
+      });
+    });
+  });
 });