Show double evaluation (#2411)

* Added tests for Show counting condition evaluations

The `<Show>` component evaluates its `when` condition more often than necessary, in particular it is immediately evaluated twice if the condition is true, children is specified as a function, and keyed is not specified.

#2406

* Fixes <Show> evaluating the condition twice

Adds another memo directly on `when` in `<Show>`.

#2406

* Made <Match> conditions only evaluate when needed

This removes a bug where the `when` condition of a `<Match>` was evaluated twice immediately after creation, when the condition was true, children was a function and keyed was not specified. It also removes any unnecessary conditions evaluations by creating a memo on every `when` in a `Switch`.

For example, if a `<Switch>` has two `<Match>`es with `when={a()}` and `when={b()}` respectively, then:
- `b()` is never called if `a()` is truthy (which was true also before this change),
- `a()` is never called when `b()` changes (which is new).

#2406

* changeset

.changeset/hot-jeans-cheer.md

@@ -0,0 +1,5 @@
+---
+"solid-js": patch
+---
+
+Removed unnecessary evaluations of <Show> and <Match> conditions.

packages/solid/src/render/flow.ts

@@ -7,7 +7,6 @@ import {
   Accessor,
   Setter,
   onCleanup,
-  MemoOptions,
   IS_DEV
 } from "../reactive/signal.js";
 import { mapArray, indexArray } from "../reactive/array.js";
@@ -106,16 +105,23 @@ export function Show<T>(props: {
   children: JSX.Element | ((item: NonNullable<T> | Accessor<NonNullable<T>>) => JSX.Element);
 }): JSX.Element {
   const keyed = props.keyed;
-  const condition = createMemo<T | undefined | null | boolean>(
+  const conditionValue = createMemo<T | undefined | null | boolean>(
     () => props.when,
     undefined,
-    IS_DEV
-      ? {
-          equals: (a, b) => (keyed ? a === b : !a === !b),
-          name: "condition"
-        }
-      : { equals: (a, b) => (keyed ? a === b : !a === !b) }
+    IS_DEV ? { name: "condition value" } : undefined
   );
+  const condition = keyed
+    ? conditionValue
+    : createMemo(
+        conditionValue,
+        undefined,
+        IS_DEV
+          ? {
+              equals: (a, b) => !a === !b,
+              name: "condition"
+            }
+          : { equals: (a, b) => !a === !b }
+      );
   return createMemo(
     () => {
       const c = condition();
@@ -129,7 +135,7 @@ export function Show<T>(props: {
                   ? (c as T)
                   : () => {
                       if (!untrack(condition)) throw narrowedError("Show");
-                      return props.when;
+                      return conditionValue();
                     }
               )
             )
@@ -142,7 +148,7 @@ export function Show<T>(props: {
   ) as unknown as JSX.Element;
 }
 
-type EvalConditions = readonly [number, unknown?, MatchProps<unknown>?];
+type EvalConditions = readonly [number, Accessor<unknown>, MatchProps<unknown>];
 
 /**
  * Switches between content based on mutually exclusive conditions
@@ -159,47 +165,58 @@ type EvalConditions = readonly [number, unknown?, MatchProps<unknown>?];
  * @description https://docs.solidjs.com/reference/components/switch-and-match
  */
 export function Switch(props: { fallback?: JSX.Element; children: JSX.Element }): JSX.Element {
-  let keyed = false;
-  const equals: MemoOptions<EvalConditions>["equals"] = (a, b) =>
-    (keyed ? a[1] === b[1] : !a[1] === !b[1]) && a[2] === b[2];
-  const conditions = children(() => props.children) as unknown as () => MatchProps<unknown>[],
-    evalConditions = createMemo(
-      (): EvalConditions => {
-        let conds = conditions();
-        if (!Array.isArray(conds)) conds = [conds];
-        for (let i = 0; i < conds.length; i++) {
-          const c = conds[i].when;
-          if (c) {
-            keyed = !!conds[i].keyed;
-            return [i, c, conds[i]];
-          }
-        }
-        return [-1];
-      },
-      undefined,
-      IS_DEV ? { equals, name: "eval conditions" } : { equals }
-    );
+  const chs = children(() => props.children);
+  const switchFunc = createMemo(() => {
+    const ch = chs() as unknown as MatchProps<unknown> | MatchProps<unknown>[];
+    const mps = Array.isArray(ch) ? ch : [ch];
+    let func: Accessor<EvalConditions | undefined> = () => undefined;
+    for (let i = 0; i < mps.length; i++) {
+      const index = i;
+      const mp = mps[i];
+      const prevFunc = func;
+      const conditionValue = createMemo(
+        () => (prevFunc() ? undefined : mp.when),
+        undefined,
+        IS_DEV ? { name: "condition value" } : undefined
+      );
+      const condition = mp.keyed
+        ? conditionValue
+        : createMemo(
+            conditionValue,
+            undefined,
+            IS_DEV
+              ? {
+                  equals: (a, b) => !a === !b,
+                  name: "condition"
+                }
+              : { equals: (a, b) => !a === !b }
+          );
+      func = () => prevFunc() || (condition() ? [index, conditionValue, mp] : undefined);
+    }
+    return func;
+  });
   return createMemo(
     () => {
-      const [index, when, cond] = evalConditions();
-      if (index < 0) return props.fallback;
-      const c = cond!.children;
-      const fn = typeof c === "function" && c.length > 0;
+      const sel = switchFunc()();
+      if (!sel) return props.fallback;
+      const [index, conditionValue, mp] = sel;
+      const child = mp.children;
+      const fn = typeof child === "function" && child.length > 0;
       return fn
         ? untrack(() =>
-            (c as any)(
-              keyed
-                ? when
+            (child as any)(
+              mp.keyed
+                ? (conditionValue() as any)
                 : () => {
-                    if (untrack(evalConditions)[0] !== index) throw narrowedError("Match");
-                    return cond!.when;
+                    if (untrack(switchFunc)()?.[0] !== index) throw narrowedError("Match");
+                    return conditionValue();
                   }
             )
           )
-        : c;
+        : child;
     },
     undefined,
-    IS_DEV ? { name: "value" } : undefined
+    IS_DEV ? { name: "eval conditions" } : undefined
   ) as unknown as JSX.Element;
 }
 

packages/solid/web/test/show.spec.tsx

@@ -72,12 +72,17 @@ describe("Testing an only child show control flow with DOM children", () => {
 describe("Testing nonkeyed show control flow", () => {
   let div!: HTMLDivElement, disposer: () => void;
   const [count, setCount] = createSignal(0);
-  let executed = 0;
+  let whenExecuted = 0;
+  let childrenExecuted = 0;
+  function when() {
+    whenExecuted++;
+    return count();
+  }
   const Component = () => (
     <div ref={div}>
-      <Show when={count()}>
+      <Show when={when()}>
         <span>{count()}</span>
-        <span>{executed++}</span>
+        <span>{childrenExecuted++}</span>
       </Show>
     </div>
   );
@@ -89,19 +94,27 @@ describe("Testing nonkeyed show control flow", () => {
     });
 
     expect(div.innerHTML).toBe("");
-    expect(executed).toBe(0);
+    expect(whenExecuted).toBe(1);
+    expect(childrenExecuted).toBe(0);
   });
 
   test("Toggle show control flow", () => {
     setCount(7);
+    expect(whenExecuted).toBe(2);
     expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("7");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
     setCount(5);
+    expect(whenExecuted).toBe(3);
     expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("5");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
+    setCount(5);
+    expect(whenExecuted).toBe(3);
     setCount(0);
+    expect(whenExecuted).toBe(4);
     expect(div.innerHTML).toBe("");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
+    setCount(5);
+    expect(whenExecuted).toBe(5);
   });
 
   test("dispose", () => disposer());
@@ -110,12 +123,17 @@ describe("Testing nonkeyed show control flow", () => {
 describe("Testing keyed show control flow", () => {
   let div!: HTMLDivElement, disposer: () => void;
   const [count, setCount] = createSignal(0);
-  let executed = 0;
+  let whenExecuted = 0;
+  let childrenExecuted = 0;
+  function when() {
+    whenExecuted++;
+    return count();
+  }
   const Component = () => (
     <div ref={div}>
-      <Show when={count()} keyed>
+      <Show when={when()} keyed>
         <span>{count()}</span>
-        <span>{executed++}</span>
+        <span>{childrenExecuted++}</span>
       </Show>
     </div>
   );
@@ -127,19 +145,27 @@ describe("Testing keyed show control flow", () => {
     });
 
     expect(div.innerHTML).toBe("");
-    expect(executed).toBe(0);
+    expect(whenExecuted).toBe(1);
+    expect(childrenExecuted).toBe(0);
   });
 
   test("Toggle show control flow", () => {
     setCount(7);
+    expect(whenExecuted).toBe(2);
     expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("7");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
     setCount(5);
+    expect(whenExecuted).toBe(3);
     expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("5");
-    expect(executed).toBe(2);
+    expect(childrenExecuted).toBe(2);
+    setCount(5);
+    expect(whenExecuted).toBe(3);
     setCount(0);
+    expect(whenExecuted).toBe(4);
     expect(div.innerHTML).toBe("");
-    expect(executed).toBe(2);
+    expect(childrenExecuted).toBe(2);
+    setCount(5);
+    expect(whenExecuted).toBe(5);
   });
 
   test("dispose", () => disposer());
@@ -148,14 +174,19 @@ describe("Testing keyed show control flow", () => {
 describe("Testing nonkeyed function show control flow", () => {
   let div!: HTMLDivElement, disposer: () => void;
   const [count, setCount] = createSignal(0);
-  let executed = 0;
+  let whenExecuted = 0;
+  let childrenExecuted = 0;
+  function when() {
+    whenExecuted++;
+    return count();
+  }
   const Component = () => (
     <div ref={div}>
-      <Show when={count()}>
+      <Show when={when()}>
         {count => (
           <>
             <span>{count()}</span>
-            <span>{executed++}</span>
+            <span>{childrenExecuted++}</span>
           </>
         )}
       </Show>
@@ -169,19 +200,27 @@ describe("Testing nonkeyed function show control flow", () => {
     });
 
     expect(div.innerHTML).toBe("");
-    expect(executed).toBe(0);
+    expect(whenExecuted).toBe(1);
+    expect(childrenExecuted).toBe(0);
   });
 
   test("Toggle show control flow", () => {
     setCount(7);
+    expect(whenExecuted).toBe(2);
     expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("7");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
     setCount(5);
+    expect(whenExecuted).toBe(3);
     expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("5");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
+    setCount(5);
+    expect(whenExecuted).toBe(3);
     setCount(0);
+    expect(whenExecuted).toBe(4);
     expect(div.innerHTML).toBe("");
-    expect(executed).toBe(1);
+    expect(childrenExecuted).toBe(1);
+    setCount(5);
+    expect(whenExecuted).toBe(5);
   });
 
   test("dispose", () => disposer());