Skip to content

Commit e7af873

Browse files
andipaetzoldandipaetzold
committed
fix: race condition in multi "*Once" hooks
1 parent d0db2af commit e7af873

File tree

2 files changed

+247
-3
lines changed

2 files changed

+247
-3
lines changed

‎src/internal/useMultiGet.spec.ts‎

Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
import { configure, renderHook, waitFor } from "@testing-library/react";
2+
import { newPromise, newSymbol } from "../__testfixtures__/index.js";
3+
import { useMultiGet } from "./useMultiGet.js";
4+
import { it, expect, beforeEach, vi, afterEach, describe } from "vitest";
5+
6+
const result1 = newSymbol<string>("Result 1");
7+
const result2 = newSymbol<string>("Result 2");
8+
9+
const refA1 = newSymbol("Ref A1");
10+
const refA2 = newSymbol("Ref A2");
11+
12+
const refB1 = newSymbol("Ref B1");
13+
const refB2 = newSymbol("Ref B2");
14+
15+
const isEqual = (a: unknown, b: unknown) =>
16+
[a, b].every((x) => [refA1, refA2].includes(x)) || [a, b].every((x) => [refB1, refB2].includes(x));
17+
18+
afterEach(() => {
19+
configure({ reactStrictMode: false });
20+
});
21+
22+
describe.each([{ reactStrictMode: true }, { reactStrictMode: false }])(
23+
`strictMode=$reactStrictMode`,
24+
({ reactStrictMode }) => {
25+
beforeEach(() => {
26+
configure({ reactStrictMode });
27+
});
28+
29+
describe("initial state", () => {
30+
it("defined reference", () => {
31+
const getData = vi.fn().mockReturnValue(new Promise(() => {}));
32+
const { result } = renderHook(() => useMultiGet([refA1], getData, isEqual));
33+
expect(result.current).toStrictEqual([[undefined, true, undefined]]);
34+
});
35+
36+
it("undefined reference", () => {
37+
const getData = vi.fn();
38+
const { result } = renderHook(() => useMultiGet([], getData, isEqual));
39+
expect(result.current).toStrictEqual([]);
40+
});
41+
42+
it("when changing ref count", () => {});
43+
});
44+
},
45+
);
46+
47+
describe("changing refs", () => {
48+
it("should not fetch twice for equal ref", async () => {
49+
const { promise, resolve } = newPromise<string>();
50+
const getData = vi.fn().mockReturnValue(promise);
51+
52+
// first ref
53+
const { result, rerender } = renderHook(({ refs }) => useMultiGet(refs, getData, isEqual), {
54+
initialProps: { refs: [refA1] },
55+
});
56+
57+
expect(getData).toHaveBeenCalledTimes(1);
58+
59+
// resolve value
60+
resolve(result1);
61+
await waitFor(() => expect(result.current).toStrictEqual([[result1, false, undefined]]));
62+
63+
// change ref
64+
rerender({ refs: [refA2] });
65+
expect(result.current).toStrictEqual([[result1, false, undefined]]);
66+
expect(getData).toHaveBeenCalledTimes(1);
67+
});
68+
69+
it("should refetch for unequal ref", async () => {
70+
const { promise: promise1, resolve: resolve1 } = newPromise<string>();
71+
const { promise: promise2, resolve: resolve2 } = newPromise<string>();
72+
const getData = vi.fn().mockReturnValueOnce(promise1).mockReturnValue(promise2);
73+
74+
// first ref
75+
const { result, rerender } = renderHook(({ refs }) => useMultiGet(refs, getData, isEqual), {
76+
initialProps: { refs: [refA1] },
77+
});
78+
expect(getData).toHaveBeenCalledTimes(1);
79+
80+
// emit value
81+
resolve1(result1);
82+
await waitFor(() => expect(result.current).toStrictEqual([[result1, false, undefined]]));
83+
84+
// change ref
85+
rerender({ refs: [refB1] });
86+
expect(result.current).toStrictEqual([[undefined, true, undefined]]);
87+
expect(getData).toHaveBeenCalledTimes(2);
88+
89+
// emit value
90+
resolve2(result2);
91+
await waitFor(() => expect(result.current).toStrictEqual([[result2, false, undefined]]));
92+
});
93+
94+
it("should ignore the first result of `getData` if the ref changes before it resolves", async () => {
95+
const { promise: promise1, resolve: resolve1 } = newPromise<string>();
96+
const { promise: promise2, resolve: resolve2 } = newPromise<string>();
97+
const getData = vi.fn().mockReturnValueOnce(promise1).mockReturnValueOnce(promise2);
98+
99+
// first ref
100+
const { result, rerender } = renderHook(({ ref }) => useMultiGet([ref], getData, isEqual), {
101+
initialProps: { ref: refA1 },
102+
});
103+
await waitFor(() => expect(result.current).toStrictEqual([[undefined, true, undefined]]));
104+
105+
// change ref
106+
rerender({ ref: refB1 });
107+
await waitFor(() => expect(result.current).toStrictEqual([[undefined, true, undefined]]));
108+
109+
// first promise resolves
110+
resolve1(result1);
111+
112+
// ensure that the first result is ignored
113+
await expect(
114+
waitFor(
115+
() => {
116+
expect(result.current).toStrictEqual([[result1, false, undefined]]);
117+
},
118+
{ timeout: 200 },
119+
),
120+
).rejects.toThrow();
121+
122+
// second promise resolves
123+
resolve2(result2);
124+
await waitFor(() => expect(result.current).toStrictEqual([[result2, false, undefined]]));
125+
});
126+
});
127+
128+
describe("changing size", () => {
129+
it("should adjust the returned states", async () => {
130+
const getData = vi.fn().mockReturnValue(new Promise(() => {}));
131+
132+
// first ref
133+
const { result, rerender } = renderHook(({ refs }) => useMultiGet(refs, getData, isEqual), {
134+
initialProps: { refs: [refA1] },
135+
});
136+
await waitFor(() => expect(result.current).toStrictEqual([[undefined, true, undefined]]));
137+
138+
// add ref
139+
rerender({ refs: [refA1, refB1] });
140+
await waitFor(() =>
141+
expect(result.current).toStrictEqual([
142+
[undefined, true, undefined],
143+
[undefined, true, undefined],
144+
]),
145+
);
146+
147+
// remove ref
148+
rerender({ refs: [refA1] });
149+
await waitFor(() => expect(result.current).toStrictEqual([[undefined, true, undefined]]));
150+
});
151+
152+
it("increase", async () => {
153+
const { promise: promise1, resolve: resolve1 } = newPromise();
154+
const { promise: promise2, resolve: resolve2 } = newPromise();
155+
const getData = vi.fn().mockReturnValueOnce(promise1).mockReturnValueOnce(promise2);
156+
157+
// first ref
158+
const { result, rerender } = renderHook(({ refs }) => useMultiGet(refs, getData, isEqual), {
159+
initialProps: { refs: [refA1] },
160+
});
161+
await waitFor(() => expect(result.current).toStrictEqual([[undefined, true, undefined]]));
162+
expect(getData).toHaveBeenCalledWith(refA1);
163+
expect(getData).toHaveBeenCalledTimes(1);
164+
165+
// add ref
166+
rerender({ refs: [refA1, refB1] });
167+
await waitFor(() =>
168+
expect(result.current).toStrictEqual([
169+
[undefined, true, undefined],
170+
[undefined, true, undefined],
171+
]),
172+
);
173+
expect(getData).toHaveBeenCalledWith(refB1);
174+
expect(getData).toHaveBeenCalledTimes(2);
175+
176+
// first promise resolves
177+
resolve1(result1);
178+
await waitFor(() =>
179+
expect(result.current).toStrictEqual([
180+
[result1, false, undefined],
181+
[undefined, true, undefined],
182+
]),
183+
);
184+
185+
// second promise resolves
186+
resolve2(result2);
187+
await waitFor(() =>
188+
expect(result.current).toStrictEqual([
189+
[result1, false, undefined],
190+
[result2, false, undefined],
191+
]),
192+
);
193+
});
194+
195+
it("decrease", async () => {
196+
const { promise: promise1, resolve: resolve1 } = newPromise();
197+
const { promise: promise2, resolve: resolve2 } = newPromise();
198+
const getData = vi.fn().mockReturnValueOnce(promise1).mockReturnValueOnce(promise2);
199+
200+
// first ref
201+
const { result, rerender } = renderHook(({ refs }) => useMultiGet(refs, getData, isEqual), {
202+
initialProps: { refs: [refA1, refB1] },
203+
});
204+
await waitFor(() =>
205+
expect(result.current).toStrictEqual([
206+
[undefined, true, undefined],
207+
[undefined, true, undefined],
208+
]),
209+
);
210+
expect(getData).toHaveBeenCalledTimes(2);
211+
212+
// second promise resolves
213+
resolve2(result2);
214+
await waitFor(() =>
215+
expect(result.current).toStrictEqual([
216+
[undefined, true, undefined],
217+
[result2, false, undefined],
218+
]),
219+
);
220+
221+
// remove ref
222+
rerender({ refs: [refA1] });
223+
await waitFor(() => expect(result.current).toStrictEqual([[undefined, true, undefined]]));
224+
expect(getData).toHaveBeenCalledTimes(2);
225+
226+
// first promise resolves
227+
resolve1(result1);
228+
await waitFor(() => expect(result.current).toStrictEqual([[result1, false, undefined]]));
229+
});
230+
});

‎src/internal/useMultiGet.ts‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,33 +15,47 @@ export function useMultiGet<Value, Error, Reference>(
1515

1616
const { states, setError, setLoading, setValue } = useMultiLoadingValue<Value, Error>(references.length);
1717
const prevReferences = useRef<Reference[]>([]);
18+
const ongoingFetchReferences = useRef<Reference[]>([]);
1819

1920
useEffect(() => {
2021
// shorten `prevReferences` size if number of references was reduced
2122
prevReferences.current = prevReferences.current.slice(0, references.length);
2223

24+
// shorten `ongoingFetchReferences` size if number of references was reduced
25+
ongoingFetchReferences.current = ongoingFetchReferences.current.slice(0, references.length);
26+
2327
// fetch to new references
2428
const changedReferences = references
2529
.map((ref, refIndex) => [ref, refIndex] as const)
2630
.filter(([ref, refIndex]) => !isEqual(ref, prevReferences.current[refIndex]));
2731

2832
for (const [ref, refIndex] of changedReferences) {
29-
(async () => {
30-
prevReferences.current[refIndex] = ref;
31-
setLoading(refIndex);
33+
prevReferences.current[refIndex] = ref;
34+
setLoading(refIndex);
35+
ongoingFetchReferences.current[refIndex] = ref;
3236

37+
(async () => {
3338
try {
3439
const data = await getData(ref);
40+
3541
if (!isMounted.current) {
3642
return;
3743
}
3844

45+
if (!isEqual(ongoingFetchReferences.current[refIndex], ref)) {
46+
return;
47+
}
48+
3949
setValue(refIndex, data);
4050
} catch (e) {
4151
if (!isMounted.current) {
4252
return;
4353
}
4454

55+
if (!isEqual(ongoingFetchReferences.current[refIndex], ref)) {
56+
return;
57+
}
58+
4559
// We assume this is always a Error
4660
setError(refIndex, e as Error);
4761
}

0 commit comments

Comments
 (0)