Skip to content

Commit 0eab77d

Browse files
TPRealTPReal
authored
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
1 parent f9ef621 commit 0eab77d

File tree

4 files changed

+231
-62
lines changed

4 files changed

+231
-62
lines changed

‎.changeset/hot-jeans-cheer.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"solid-js": patch
3+
---
4+
5+
Removed unnecessary evaluations of <Show> and <Match> conditions.

‎packages/solid/src/render/flow.ts‎

Lines changed: 58 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import {
77
Accessor,
88
Setter,
99
onCleanup,
10-
MemoOptions,
1110
IS_DEV
1211
} from "../reactive/signal.js";
1312
import { mapArray, indexArray } from "../reactive/array.js";
@@ -106,16 +105,23 @@ export function Show<T>(props: {
106105
children: JSX.Element | ((item: NonNullable<T> | Accessor<NonNullable<T>>) => JSX.Element);
107106
}): JSX.Element {
108107
const keyed = props.keyed;
109-
const condition = createMemo<T | undefined | null | boolean>(
108+
const conditionValue = createMemo<T | undefined | null | boolean>(
110109
() => props.when,
111110
undefined,
112-
IS_DEV
113-
? {
114-
equals: (a, b) => (keyed ? a === b : !a === !b),
115-
name: "condition"
116-
}
117-
: { equals: (a, b) => (keyed ? a === b : !a === !b) }
111+
IS_DEV ? { name: "condition value" } : undefined
118112
);
113+
const condition = keyed
114+
? conditionValue
115+
: createMemo(
116+
conditionValue,
117+
undefined,
118+
IS_DEV
119+
? {
120+
equals: (a, b) => !a === !b,
121+
name: "condition"
122+
}
123+
: { equals: (a, b) => !a === !b }
124+
);
119125
return createMemo(
120126
() => {
121127
const c = condition();
@@ -129,7 +135,7 @@ export function Show<T>(props: {
129135
? (c as T)
130136
: () => {
131137
if (!untrack(condition)) throw narrowedError("Show");
132-
return props.when;
138+
return conditionValue();
133139
}
134140
)
135141
)
@@ -142,7 +148,7 @@ export function Show<T>(props: {
142148
) as unknown as JSX.Element;
143149
}
144150

145-
type EvalConditions = readonly [number, unknown?, MatchProps<unknown>?];
151+
type EvalConditions = readonly [number, Accessor<unknown>, MatchProps<unknown>];
146152

147153
/**
148154
* Switches between content based on mutually exclusive conditions
@@ -159,47 +165,58 @@ type EvalConditions = readonly [number, unknown?, MatchProps<unknown>?];
159165
* @description https://docs.solidjs.com/reference/components/switch-and-match
160166
*/
161167
export function Switch(props: { fallback?: JSX.Element; children: JSX.Element }): JSX.Element {
162-
let keyed = false;
163-
const equals: MemoOptions<EvalConditions>["equals"] = (a, b) =>
164-
(keyed ? a[1] === b[1] : !a[1] === !b[1]) && a[2] === b[2];
165-
const conditions = children(() => props.children) as unknown as () => MatchProps<unknown>[],
166-
evalConditions = createMemo(
167-
(): EvalConditions => {
168-
let conds = conditions();
169-
if (!Array.isArray(conds)) conds = [conds];
170-
for (let i = 0; i < conds.length; i++) {
171-
const c = conds[i].when;
172-
if (c) {
173-
keyed = !!conds[i].keyed;
174-
return [i, c, conds[i]];
175-
}
176-
}
177-
return [-1];
178-
},
179-
undefined,
180-
IS_DEV ? { equals, name: "eval conditions" } : { equals }
181-
);
168+
const chs = children(() => props.children);
169+
const switchFunc = createMemo(() => {
170+
const ch = chs() as unknown as MatchProps<unknown> | MatchProps<unknown>[];
171+
const mps = Array.isArray(ch) ? ch : [ch];
172+
let func: Accessor<EvalConditions | undefined> = () => undefined;
173+
for (let i = 0; i < mps.length; i++) {
174+
const index = i;
175+
const mp = mps[i];
176+
const prevFunc = func;
177+
const conditionValue = createMemo(
178+
() => (prevFunc() ? undefined : mp.when),
179+
undefined,
180+
IS_DEV ? { name: "condition value" } : undefined
181+
);
182+
const condition = mp.keyed
183+
? conditionValue
184+
: createMemo(
185+
conditionValue,
186+
undefined,
187+
IS_DEV
188+
? {
189+
equals: (a, b) => !a === !b,
190+
name: "condition"
191+
}
192+
: { equals: (a, b) => !a === !b }
193+
);
194+
func = () => prevFunc() || (condition() ? [index, conditionValue, mp] : undefined);
195+
}
196+
return func;
197+
});
182198
return createMemo(
183199
() => {
184-
const [index, when, cond] = evalConditions();
185-
if (index < 0) return props.fallback;
186-
const c = cond!.children;
187-
const fn = typeof c === "function" && c.length > 0;
200+
const sel = switchFunc()();
201+
if (!sel) return props.fallback;
202+
const [index, conditionValue, mp] = sel;
203+
const child = mp.children;
204+
const fn = typeof child === "function" && child.length > 0;
188205
return fn
189206
? untrack(() =>
190-
(c as any)(
191-
keyed
192-
? when
207+
(child as any)(
208+
mp.keyed
209+
? (conditionValue() as any)
193210
: () => {
194-
if (untrack(evalConditions)[0] !== index) throw narrowedError("Match");
195-
return cond!.when;
211+
if (untrack(switchFunc)()?.[0] !== index) throw narrowedError("Match");
212+
return conditionValue();
196213
}
197214
)
198215
)
199-
: c;
216+
: child;
200217
},
201218
undefined,
202-
IS_DEV ? { name: "value" } : undefined
219+
IS_DEV ? { name: "eval conditions" } : undefined
203220
) as unknown as JSX.Element;
204221
}
205222

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

Lines changed: 60 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,17 @@ describe("Testing an only child show control flow with DOM children", () => {
7272
describe("Testing nonkeyed show control flow", () => {
7373
let div!: HTMLDivElement, disposer: () => void;
7474
const [count, setCount] = createSignal(0);
75-
let executed = 0;
75+
let whenExecuted = 0;
76+
let childrenExecuted = 0;
77+
function when() {
78+
whenExecuted++;
79+
return count();
80+
}
7681
const Component = () => (
7782
<div ref={div}>
78-
<Show when={count()}>
83+
<Show when={when()}>
7984
<span>{count()}</span>
80-
<span>{executed++}</span>
85+
<span>{childrenExecuted++}</span>
8186
</Show>
8287
</div>
8388
);
@@ -89,19 +94,27 @@ describe("Testing nonkeyed show control flow", () => {
8994
});
9095

9196
expect(div.innerHTML).toBe("");
92-
expect(executed).toBe(0);
97+
expect(whenExecuted).toBe(1);
98+
expect(childrenExecuted).toBe(0);
9399
});
94100

95101
test("Toggle show control flow", () => {
96102
setCount(7);
103+
expect(whenExecuted).toBe(2);
97104
expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("7");
98-
expect(executed).toBe(1);
105+
expect(childrenExecuted).toBe(1);
99106
setCount(5);
107+
expect(whenExecuted).toBe(3);
100108
expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("5");
101-
expect(executed).toBe(1);
109+
expect(childrenExecuted).toBe(1);
110+
setCount(5);
111+
expect(whenExecuted).toBe(3);
102112
setCount(0);
113+
expect(whenExecuted).toBe(4);
103114
expect(div.innerHTML).toBe("");
104-
expect(executed).toBe(1);
115+
expect(childrenExecuted).toBe(1);
116+
setCount(5);
117+
expect(whenExecuted).toBe(5);
105118
});
106119

107120
test("dispose", () => disposer());
@@ -110,12 +123,17 @@ describe("Testing nonkeyed show control flow", () => {
110123
describe("Testing keyed show control flow", () => {
111124
let div!: HTMLDivElement, disposer: () => void;
112125
const [count, setCount] = createSignal(0);
113-
let executed = 0;
126+
let whenExecuted = 0;
127+
let childrenExecuted = 0;
128+
function when() {
129+
whenExecuted++;
130+
return count();
131+
}
114132
const Component = () => (
115133
<div ref={div}>
116-
<Show when={count()} keyed>
134+
<Show when={when()} keyed>
117135
<span>{count()}</span>
118-
<span>{executed++}</span>
136+
<span>{childrenExecuted++}</span>
119137
</Show>
120138
</div>
121139
);
@@ -127,19 +145,27 @@ describe("Testing keyed show control flow", () => {
127145
});
128146

129147
expect(div.innerHTML).toBe("");
130-
expect(executed).toBe(0);
148+
expect(whenExecuted).toBe(1);
149+
expect(childrenExecuted).toBe(0);
131150
});
132151

133152
test("Toggle show control flow", () => {
134153
setCount(7);
154+
expect(whenExecuted).toBe(2);
135155
expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("7");
136-
expect(executed).toBe(1);
156+
expect(childrenExecuted).toBe(1);
137157
setCount(5);
158+
expect(whenExecuted).toBe(3);
138159
expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("5");
139-
expect(executed).toBe(2);
160+
expect(childrenExecuted).toBe(2);
161+
setCount(5);
162+
expect(whenExecuted).toBe(3);
140163
setCount(0);
164+
expect(whenExecuted).toBe(4);
141165
expect(div.innerHTML).toBe("");
142-
expect(executed).toBe(2);
166+
expect(childrenExecuted).toBe(2);
167+
setCount(5);
168+
expect(whenExecuted).toBe(5);
143169
});
144170

145171
test("dispose", () => disposer());
@@ -148,14 +174,19 @@ describe("Testing keyed show control flow", () => {
148174
describe("Testing nonkeyed function show control flow", () => {
149175
let div!: HTMLDivElement, disposer: () => void;
150176
const [count, setCount] = createSignal(0);
151-
let executed = 0;
177+
let whenExecuted = 0;
178+
let childrenExecuted = 0;
179+
function when() {
180+
whenExecuted++;
181+
return count();
182+
}
152183
const Component = () => (
153184
<div ref={div}>
154-
<Show when={count()}>
185+
<Show when={when()}>
155186
{count => (
156187
<>
157188
<span>{count()}</span>
158-
<span>{executed++}</span>
189+
<span>{childrenExecuted++}</span>
159190
</>
160191
)}
161192
</Show>
@@ -169,19 +200,27 @@ describe("Testing nonkeyed function show control flow", () => {
169200
});
170201

171202
expect(div.innerHTML).toBe("");
172-
expect(executed).toBe(0);
203+
expect(whenExecuted).toBe(1);
204+
expect(childrenExecuted).toBe(0);
173205
});
174206

175207
test("Toggle show control flow", () => {
176208
setCount(7);
209+
expect(whenExecuted).toBe(2);
177210
expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("7");
178-
expect(executed).toBe(1);
211+
expect(childrenExecuted).toBe(1);
179212
setCount(5);
213+
expect(whenExecuted).toBe(3);
180214
expect((div.firstChild as HTMLSpanElement).innerHTML).toBe("5");
181-
expect(executed).toBe(1);
215+
expect(childrenExecuted).toBe(1);
216+
setCount(5);
217+
expect(whenExecuted).toBe(3);
182218
setCount(0);
219+
expect(whenExecuted).toBe(4);
183220
expect(div.innerHTML).toBe("");
184-
expect(executed).toBe(1);
221+
expect(childrenExecuted).toBe(1);
222+
setCount(5);
223+
expect(whenExecuted).toBe(5);
185224
});
186225

187226
test("dispose", () => disposer());

0 commit comments

Comments
 (0)