fix: repeat must not mutate its result object in place (#785) (#787)

Author: thetarnav

.changeset/tender-mirrors-lay.md

@@ -0,0 +1,5 @@
+---
+"@solid-primitives/range": patch
+---
+
+repeat now returns clones of its result instead of mutating

packages/range/src/repeat.ts

@@ -1,4 +1,4 @@
-import { Accessor, JSX, createMemo, createRoot, onCleanup, untrack } from "solid-js";
+import { Accessor, JSX, createMemo, createRoot, onCleanup } from "solid-js";
 import { toFunction } from "./common.js";
 
 /**
@@ -23,60 +23,68 @@ export function repeat<T>(
   mapFn: (i: number) => T,
   options: { fallback?: Accessor<T> } = {},
 ): Accessor<T[]> {
-  let disposers: (() => void)[] = [],
-    items: T[] = [],
-    prevLen = 0;
-
-  onCleanup(() => disposers.forEach(f => f()));
+  let prev: readonly T[] = [];
+  let prevLen: number | undefined;
+  const disposers: (() => void)[] = [];
+  onCleanup(() => {
+    for (let index = 0; index < disposers.length; index++) {
+      disposers[index]!();
+    }
+  });
 
-  const mapLength = (len: number): T[] => {
-    if (len === 0) {
-      disposers.forEach(f => f());
+  // Truncate toward zero and force positive
+  const memoLen = createMemo(() => Math.max(times() | 0, 0));
 
-      if (options.fallback)
-        return createRoot(dispose => {
-          disposers = [dispose];
-          return (items = [options.fallback!()]);
-        });
+  return function mapLength(): T[] {
+    const len = memoLen();
+    if (len === prevLen) return prev as T[];
 
-      disposers = [];
-      return (items = []);
+    // Dispose of fallback or unnecessarry elements
+    if (prevLen === 0) disposers[0]?.();
+    else {
+      for (let index = len; index < disposers.length; index++) {
+        disposers[index]!();
+      }
     }
 
-    if (prevLen === 0) {
-      // after fallback case:
-      if (disposers[0]) disposers[0]();
-      for (let i = 0; i < len; i++) items[i] = createRoot(mapper.bind(void 0, i));
-      return items;
-    }
+    // The following prefers to use `prev.slice` to
+    // preserve any array element kind optimizations
+    // the runtime has made.
 
-    {
-      const diff = prevLen - len;
-      if (diff > 0) {
-        for (let i = prevLen - 1; i >= len; i--) disposers[i]!();
-        items.splice(len, diff);
-        disposers.splice(len, diff);
-        return items;
+    if (len === 0) {
+      const fallback = options.fallback;
+      if (fallback) {
+        // Show fallback if available
+        const next = prev.slice(0, 1);
+        next[0] = createRoot(dispose => {
+          disposers[0] = dispose;
+          return fallback();
+        });
+
+        disposers.length = 1;
+        prevLen = 0;
+        return (prev = next);
+      } else {
+        // Show empty array, otherwise
+        disposers.length = 0;
+        prevLen = 0;
+        return (prev = prev.slice(0, 0));
       }
     }
 
-    for (let i = prevLen; i < len; i++) items[i] = createRoot(mapper.bind(void 0, i));
-    return items;
-  };
+    const next = prev.slice(0, len);
 
-  const mapper = (index: number, dispose: () => void): T => {
-    disposers[index] = dispose;
-    return mapFn(index);
-  };
+    // Create new elements as needed
+    for (let index = prevLen ?? 0; index < len; index++) {
+      next[index] = createRoot(dispose => {
+        disposers[index] = dispose;
+        return mapFn(index);
+      });
+    }
 
-  const memoLen = createMemo(() => Math.floor(Math.max(times(), 0)));
-  return () => {
-    const len = memoLen();
-    return untrack(() => {
-      const newItems = mapLength(len);
-      prevLen = len;
-      return newItems;
-    });
+    disposers.length = len;
+    prevLen = len;
+    return (prev = next);
   };
 }
 

packages/range/test/repeat.test.ts

@@ -1,6 +1,6 @@
 import { expect, describe, it } from "vitest";
 import { createComputed, createRoot, createSignal, onCleanup } from "solid-js";
-import { repeat } from "../src/index.js";
+import { Repeat, repeat } from "../src/index.js";
 
 describe("repeat", () => {
   it("maps only added items", () =>
@@ -82,4 +82,52 @@ describe("repeat", () => {
       setLength(3);
       expect(mapped(), "mapped after dispose").toEqual(["fb"]);
     }));
+
+  it("uses fallback when length is initially 0", () =>
+    createRoot(disposer => {
+      const map = repeat(
+        () => 0,
+        i => i,
+        { fallback: () => NaN },
+      );
+      expect(map()).toEqual([NaN]);
+      disposer();
+    }));
+});
+
+describe("<Repeat/>", () => {
+  it("notifies observers on length change", () => {
+    const [length, setLength] = createSignal(3);
+
+    const [dispose, accessor] = createRoot(dispose => {
+      const accessor = Repeat({
+        get times() {
+          return length();
+        },
+        fallback: () => 0,
+        children: () => 1,
+      }) as never as () => {};
+      return [dispose, accessor];
+    });
+
+    let notifications = 0;
+    createComputed(() => {
+      accessor();
+      notifications++;
+    });
+
+    expect(notifications).toEqual(1);
+    setLength(4);
+    expect(notifications).toEqual(2);
+    setLength(0);
+    expect(notifications).toEqual(3);
+    setLength(2);
+    expect(notifications).toEqual(4);
+    setLength(1);
+    expect(notifications).toEqual(5);
+    setLength(1.5);
+    expect(notifications).toEqual(5);
+
+    dispose();
+  });
 });