From b3d3815826590a035a68a6b70f2551955da0662a Mon Sep 17 00:00:00 2001 From: webstrand Date: Sun, 20 Apr 2025 13:35:58 -0400 Subject: [PATCH 1/2] fix: repeat must not mutate its result object in place (#785) --- .changeset/tender-mirrors-lay.md | 5 ++ packages/range/src/repeat.ts | 96 ++++++++++++++++-------------- packages/range/test/repeat.test.ts | 39 +++++++++++- 3 files changed, 95 insertions(+), 45 deletions(-) create mode 100644 .changeset/tender-mirrors-lay.md diff --git a/.changeset/tender-mirrors-lay.md b/.changeset/tender-mirrors-lay.md new file mode 100644 index 000000000..b8d7080eb --- /dev/null +++ b/.changeset/tender-mirrors-lay.md @@ -0,0 +1,5 @@ +--- +"@solid-primitives/range": patch +--- + +repeat now returns clones of its result instead of mutating diff --git a/packages/range/src/repeat.ts b/packages/range/src/repeat.ts index d79efa7c2..46eeca6db 100644 --- a/packages/range/src/repeat.ts +++ b/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( mapFn: (i: number) => T, options: { fallback?: Accessor } = {}, ): Accessor { - let disposers: (() => void)[] = [], - items: T[] = [], - prevLen = 0; - - onCleanup(() => disposers.forEach(f => f())); + let prev: readonly T[] = []; + let prevLen = 0; + 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; 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); }; } diff --git a/packages/range/test/repeat.test.ts b/packages/range/test/repeat.test.ts index c23cb3690..374d189e0 100644 --- a/packages/range/test/repeat.test.ts +++ b/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", () => @@ -83,3 +83,40 @@ describe("repeat", () => { expect(mapped(), "mapped after dispose").toEqual(["fb"]); })); }); + +describe("", () => { + 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(); + }); +}); From 0192f344249d1d069c72ecc926d24ce505929bef Mon Sep 17 00:00:00 2001 From: webstrand Date: Wed, 23 Apr 2025 19:16:39 -0400 Subject: [PATCH 2/2] fix: repeat does not use fallback on initial length 0 --- packages/range/src/repeat.ts | 4 ++-- packages/range/test/repeat.test.ts | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/range/src/repeat.ts b/packages/range/src/repeat.ts index 46eeca6db..51b21e1e9 100644 --- a/packages/range/src/repeat.ts +++ b/packages/range/src/repeat.ts @@ -24,7 +24,7 @@ export function repeat( options: { fallback?: Accessor } = {}, ): Accessor { let prev: readonly T[] = []; - let prevLen = 0; + let prevLen: number | undefined; const disposers: (() => void)[] = []; onCleanup(() => { for (let index = 0; index < disposers.length; index++) { @@ -75,7 +75,7 @@ export function repeat( const next = prev.slice(0, len); // Create new elements as needed - for (let index = prevLen; index < len; index++) { + for (let index = prevLen ?? 0; index < len; index++) { next[index] = createRoot(dispose => { disposers[index] = dispose; return mapFn(index); diff --git a/packages/range/test/repeat.test.ts b/packages/range/test/repeat.test.ts index 374d189e0..e98ab5807 100644 --- a/packages/range/test/repeat.test.ts +++ b/packages/range/test/repeat.test.ts @@ -82,6 +82,17 @@ 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("", () => {