Skip to content

Commit d4e24b3

Browse files
authored
[autodeps] Do not include nonreactive refs or setStates in inferred deps (facebook#32236)
1 parent bdce84a commit d4e24b3

File tree

11 files changed

+301
-3
lines changed

11 files changed

+301
-3
lines changed

compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import {
1515
ReactiveScopeDependency,
1616
Place,
1717
ReactiveScopeDependencies,
18+
isUseRefType,
19+
isSetStateType,
1820
} from '../HIR';
1921
import {DEFAULT_EXPORT} from '../HIR/Environment';
2022
import {
@@ -181,11 +183,20 @@ export function inferEffectDependencies(fn: HIRFunction): void {
181183
/**
182184
* Step 1: push dependencies to the effect deps array
183185
*
184-
* Note that it's invalid to prune non-reactive deps in this pass, see
186+
* Note that it's invalid to prune all non-reactive deps in this pass, see
185187
* the `infer-effect-deps/pruned-nonreactive-obj` fixture for an
186188
* explanation.
187189
*/
188190
for (const dep of scopeInfo.deps) {
191+
if (
192+
(isUseRefType(dep.identifier) ||
193+
isSetStateType(dep.identifier)) &&
194+
!reactiveIds.has(dep.identifier.id)
195+
) {
196+
// exclude non-reactive hook results, which will never be in a memo block
197+
continue;
198+
}
199+
189200
const {place, instructions} = writeDependencyToInstructions(
190201
dep,
191202
reactiveIds.has(dep.identifier.id),
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies
6+
import {useEffect, useRef} from 'react';
7+
function useCustomRef() {
8+
const ref = useRef();
9+
return ref;
10+
}
11+
function NonReactiveWrapper() {
12+
const ref = useCustomRef();
13+
useEffect(() => {
14+
print(ref);
15+
});
16+
}
17+
18+
```
19+
20+
## Code
21+
22+
```javascript
23+
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
24+
import { useEffect, useRef } from "react";
25+
function useCustomRef() {
26+
const ref = useRef();
27+
return ref;
28+
}
29+
30+
function NonReactiveWrapper() {
31+
const $ = _c(2);
32+
const ref = useCustomRef();
33+
let t0;
34+
if ($[0] !== ref) {
35+
t0 = () => {
36+
print(ref);
37+
};
38+
$[0] = ref;
39+
$[1] = t0;
40+
} else {
41+
t0 = $[1];
42+
}
43+
useEffect(t0, [ref]);
44+
}
45+
46+
```
47+
48+
### Eval output
49+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// @inferEffectDependencies
2+
import {useEffect, useRef} from 'react';
3+
function useCustomRef() {
4+
const ref = useRef();
5+
return ref;
6+
}
7+
function NonReactiveWrapper() {
8+
const ref = useCustomRef();
9+
useEffect(() => {
10+
print(ref);
11+
});
12+
}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ function Component(t0) {
101101
useEffect(t3, [
102102
foo,
103103
bar,
104-
ref,
105104
localNonPrimitiveReactive,
106105
localNonPrimitiveNonreactive,
107106
]);

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/nonreactive-ref.expect.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ function NonReactiveRefInEffect() {
4242
} else {
4343
t0 = $[0];
4444
}
45-
useEffect(t0, [ref]);
45+
useEffect(t0, []);
4646
}
4747

4848
```
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies
6+
import {useEffect, useState} from 'react';
7+
import {print} from 'shared-runtime';
8+
9+
/**
10+
* Special case of `infer-effect-deps/nonreactive-dep`.
11+
*
12+
* We know that local `useRef` return values are stable, regardless of
13+
* inferred memoization.
14+
*/
15+
function NonReactiveSetStateInEffect() {
16+
const [_, setState] = useState('initial value');
17+
useEffect(() => print(setState));
18+
}
19+
20+
```
21+
22+
## Code
23+
24+
```javascript
25+
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
26+
import { useEffect, useState } from "react";
27+
import { print } from "shared-runtime";
28+
29+
/**
30+
* Special case of `infer-effect-deps/nonreactive-dep`.
31+
*
32+
* We know that local `useRef` return values are stable, regardless of
33+
* inferred memoization.
34+
*/
35+
function NonReactiveSetStateInEffect() {
36+
const $ = _c(1);
37+
const [, setState] = useState("initial value");
38+
let t0;
39+
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
40+
t0 = () => print(setState);
41+
$[0] = t0;
42+
} else {
43+
t0 = $[0];
44+
}
45+
useEffect(t0, []);
46+
}
47+
48+
```
49+
50+
### Eval output
51+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// @inferEffectDependencies
2+
import {useEffect, useState} from 'react';
3+
import {print} from 'shared-runtime';
4+
5+
/**
6+
* Special case of `infer-effect-deps/nonreactive-dep`.
7+
*
8+
* We know that local `useRef` return values are stable, regardless of
9+
* inferred memoization.
10+
*/
11+
function NonReactiveSetStateInEffect() {
12+
const [_, setState] = useState('initial value');
13+
useEffect(() => print(setState));
14+
}
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies
6+
import {useEffect, useRef} from 'react';
7+
import {print} from 'shared-runtime';
8+
9+
/*
10+
* Ref types are not enough to determine to omit from deps. Must also take reactivity into account.
11+
*/
12+
function ReactiveRefInEffect(props) {
13+
const ref1 = useRef('initial value');
14+
const ref2 = useRef('initial value');
15+
let ref;
16+
if (props.foo) {
17+
ref = ref1;
18+
} else {
19+
ref = ref2;
20+
}
21+
useEffect(() => print(ref));
22+
}
23+
24+
```
25+
26+
## Code
27+
28+
```javascript
29+
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
30+
import { useEffect, useRef } from "react";
31+
import { print } from "shared-runtime";
32+
33+
/*
34+
* Ref types are not enough to determine to omit from deps. Must also take reactivity into account.
35+
*/
36+
function ReactiveRefInEffect(props) {
37+
const $ = _c(4);
38+
const ref1 = useRef("initial value");
39+
const ref2 = useRef("initial value");
40+
let ref;
41+
if ($[0] !== props.foo) {
42+
if (props.foo) {
43+
ref = ref1;
44+
} else {
45+
ref = ref2;
46+
}
47+
$[0] = props.foo;
48+
$[1] = ref;
49+
} else {
50+
ref = $[1];
51+
}
52+
let t0;
53+
if ($[2] !== ref) {
54+
t0 = () => print(ref);
55+
$[2] = ref;
56+
$[3] = t0;
57+
} else {
58+
t0 = $[3];
59+
}
60+
useEffect(t0, [ref]);
61+
}
62+
63+
```
64+
65+
### Eval output
66+
(kind: exception) Fixture not implemented
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// @inferEffectDependencies
2+
import {useEffect, useRef} from 'react';
3+
import {print} from 'shared-runtime';
4+
5+
/*
6+
* Ref types are not enough to determine to omit from deps. Must also take reactivity into account.
7+
*/
8+
function ReactiveRefInEffect(props) {
9+
const ref1 = useRef('initial value');
10+
const ref2 = useRef('initial value');
11+
let ref;
12+
if (props.foo) {
13+
ref = ref1;
14+
} else {
15+
ref = ref2;
16+
}
17+
useEffect(() => print(ref));
18+
}
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @inferEffectDependencies
6+
import {useEffect, useState} from 'react';
7+
import {print} from 'shared-runtime';
8+
9+
/*
10+
* setState types are not enough to determine to omit from deps. Must also take reactivity into account.
11+
*/
12+
function ReactiveRefInEffect(props) {
13+
const [_state1, setState1] = useRef('initial value');
14+
const [_state2, setState2] = useRef('initial value');
15+
let setState;
16+
if (props.foo) {
17+
setState = setState1;
18+
} else {
19+
setState = setState2;
20+
}
21+
useEffect(() => print(setState));
22+
}
23+
24+
```
25+
26+
## Code
27+
28+
```javascript
29+
import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies
30+
import { useEffect, useState } from "react";
31+
import { print } from "shared-runtime";
32+
33+
/*
34+
* setState types are not enough to determine to omit from deps. Must also take reactivity into account.
35+
*/
36+
function ReactiveRefInEffect(props) {
37+
const $ = _c(2);
38+
const [, setState1] = useRef("initial value");
39+
const [, setState2] = useRef("initial value");
40+
let setState;
41+
if (props.foo) {
42+
setState = setState1;
43+
} else {
44+
setState = setState2;
45+
}
46+
let t0;
47+
if ($[0] !== setState) {
48+
t0 = () => print(setState);
49+
$[0] = setState;
50+
$[1] = t0;
51+
} else {
52+
t0 = $[1];
53+
}
54+
useEffect(t0, [setState]);
55+
}
56+
57+
```
58+
59+
### Eval output
60+
(kind: exception) Fixture not implemented

0 commit comments

Comments
 (0)