Skip to content

Commit 454e01e

Browse files
authored
[compiler] Allow manual dependencies to have different optionality than inferred deps (facebook#35186)
Since adding this validation we've already changed our inference to use knowledge from manual memoization to inform when values are frozen and which values are non-nullable. To align with that, if the user chooses to use different optionality btw the deps and the memo block/callback, that's fine. The key is that eg `x?.y` will invalidate whenever `x.y` would, so from a memoization correctness perspective its fine. It's not our job to be a type checker: if a value is potentially nullable, it should likely use a nullable property access in both places but TypeScript/Flow can check that. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35186). * facebook#35201 * facebook#35202 * facebook#35192 * facebook#35190 * __->__ facebook#35186
1 parent c9a8cf3 commit 454e01e

File tree

3 files changed

+44
-39
lines changed

3 files changed

+44
-39
lines changed

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {
2424
InstructionKind,
2525
isStableType,
2626
isSubPath,
27+
isSubPathIgnoringOptionals,
2728
isUseRefType,
2829
LoadGlobal,
2930
ManualMemoDependency,
@@ -240,7 +241,10 @@ export function validateExhaustiveDependencies(
240241
manualDependency.root.value.identifier.id ===
241242
inferredDependency.identifier.id &&
242243
(areEqualPaths(manualDependency.path, inferredDependency.path) ||
243-
isSubPath(manualDependency.path, inferredDependency.path))
244+
isSubPathIgnoringOptionals(
245+
manualDependency.path,
246+
inferredDependency.path,
247+
))
244248
) {
245249
hasMatchingManualDependency = true;
246250
matched.add(manualDependency);

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md

Lines changed: 32 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -9,23 +9,29 @@ import {Stringify} from 'shared-runtime';
99
function Component({x, y, z}) {
1010
const a = useMemo(() => {
1111
return x?.y.z?.a;
12+
// error: too precise
1213
}, [x?.y.z?.a.b]);
1314
const b = useMemo(() => {
1415
return x.y.z?.a;
16+
// ok, not our job to type check nullability
1517
}, [x.y.z.a]);
1618
const c = useMemo(() => {
1719
return x?.y.z.a?.b;
20+
// error: too precise
1821
}, [x?.y.z.a?.b.z]);
1922
const d = useMemo(() => {
2023
return x?.y?.[(console.log(y), z?.b)];
24+
// ok
2125
}, [x?.y, y, z?.b]);
2226
const e = useMemo(() => {
2327
const e = [];
2428
e.push(x);
2529
return e;
30+
// ok
2631
}, [x]);
2732
const f = useMemo(() => {
2833
return [];
34+
// error: unnecessary
2935
}, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
3036
const ref1 = useRef(null);
3137
const ref2 = useRef(null);
@@ -34,6 +40,7 @@ function Component({x, y, z}) {
3440
return () => {
3541
return ref.current;
3642
};
43+
// error: ref is a stable type but reactive
3744
}, []);
3845
return <Stringify results={[a, b, c, d, e, f, cb]} />;
3946
}
@@ -44,7 +51,7 @@ function Component({x, y, z}) {
4451
## Error
4552
4653
```
47-
Found 5 errors:
54+
Found 4 errors:
4855

4956
Error: Found non-exhaustive dependencies
5057

@@ -55,61 +62,48 @@ error.invalid-exhaustive-deps.ts:7:11
5562
6 | const a = useMemo(() => {
5663
> 7 | return x?.y.z?.a;
5764
| ^^^^^^^^^ Missing dependency `x?.y.z?.a`
58-
8 | }, [x?.y.z?.a.b]);
59-
9 | const b = useMemo(() => {
60-
10 | return x.y.z?.a;
65+
8 | // error: too precise
66+
9 | }, [x?.y.z?.a.b]);
67+
10 | const b = useMemo(() => {
6168

6269
Error: Found non-exhaustive dependencies
6370

6471
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
6572

66-
error.invalid-exhaustive-deps.ts:10:11
67-
8 | }, [x?.y.z?.a.b]);
68-
9 | const b = useMemo(() => {
69-
> 10 | return x.y.z?.a;
70-
| ^^^^^^^^ Missing dependency `x.y.z?.a`
71-
11 | }, [x.y.z.a]);
72-
12 | const c = useMemo(() => {
73-
13 | return x?.y.z.a?.b;
74-
75-
Error: Found non-exhaustive dependencies
76-
77-
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
78-
79-
error.invalid-exhaustive-deps.ts:13:11
80-
11 | }, [x.y.z.a]);
81-
12 | const c = useMemo(() => {
82-
> 13 | return x?.y.z.a?.b;
73+
error.invalid-exhaustive-deps.ts:15:11
74+
13 | }, [x.y.z.a]);
75+
14 | const c = useMemo(() => {
76+
> 15 | return x?.y.z.a?.b;
8377
| ^^^^^^^^^^^ Missing dependency `x?.y.z.a?.b`
84-
14 | }, [x?.y.z.a?.b.z]);
85-
15 | const d = useMemo(() => {
86-
16 | return x?.y?.[(console.log(y), z?.b)];
78+
16 | // error: too precise
79+
17 | }, [x?.y.z.a?.b.z]);
80+
18 | const d = useMemo(() => {
8781

8882
Error: Found unnecessary memoization dependencies
8983

9084
Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected.
9185

92-
error.invalid-exhaustive-deps.ts:25:5
93-
23 | const f = useMemo(() => {
94-
24 | return [];
95-
> 25 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
86+
error.invalid-exhaustive-deps.ts:31:5
87+
29 | return [];
88+
30 | // error: unnecessary
89+
> 31 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
9690
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL`
97-
26 | const ref1 = useRef(null);
98-
27 | const ref2 = useRef(null);
99-
28 | const ref = z ? ref1 : ref2;
91+
32 | const ref1 = useRef(null);
92+
33 | const ref2 = useRef(null);
93+
34 | const ref = z ? ref1 : ref2;
10094

10195
Error: Found non-exhaustive dependencies
10296

10397
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
10498

105-
error.invalid-exhaustive-deps.ts:31:13
106-
29 | const cb = useMemo(() => {
107-
30 | return () => {
108-
> 31 | return ref.current;
99+
error.invalid-exhaustive-deps.ts:37:13
100+
35 | const cb = useMemo(() => {
101+
36 | return () => {
102+
> 37 | return ref.current;
109103
| ^^^ Missing dependency `ref`. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values
110-
32 | };
111-
33 | }, []);
112-
34 | return <Stringify results={[a, b, c, d, e, f, cb]} />;
104+
38 | };
105+
39 | // error: ref is a stable type but reactive
106+
40 | }, []);
113107
```
114108
115109

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,23 +5,29 @@ import {Stringify} from 'shared-runtime';
55
function Component({x, y, z}) {
66
const a = useMemo(() => {
77
return x?.y.z?.a;
8+
// error: too precise
89
}, [x?.y.z?.a.b]);
910
const b = useMemo(() => {
1011
return x.y.z?.a;
12+
// ok, not our job to type check nullability
1113
}, [x.y.z.a]);
1214
const c = useMemo(() => {
1315
return x?.y.z.a?.b;
16+
// error: too precise
1417
}, [x?.y.z.a?.b.z]);
1518
const d = useMemo(() => {
1619
return x?.y?.[(console.log(y), z?.b)];
20+
// ok
1721
}, [x?.y, y, z?.b]);
1822
const e = useMemo(() => {
1923
const e = [];
2024
e.push(x);
2125
return e;
26+
// ok
2227
}, [x]);
2328
const f = useMemo(() => {
2429
return [];
30+
// error: unnecessary
2531
}, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
2632
const ref1 = useRef(null);
2733
const ref2 = useRef(null);
@@ -30,6 +36,7 @@ function Component({x, y, z}) {
3036
return () => {
3137
return ref.current;
3238
};
39+
// error: ref is a stable type but reactive
3340
}, []);
3441
return <Stringify results={[a, b, c, d, e, f, cb]} />;
3542
}

0 commit comments

Comments
 (0)