Skip to content

Commit 257b033

Browse files
authored
[Compiler] Avoid capturing global setStates for no-derived-computations lint (facebook#35135)
Summary: This only matters when enableTreatSetIdentifiersAsStateSetters=true This pattern is still bad. But Right now the validation can only recommend to move stuff to "calculate in render" A global setState should not be moved to render, not even conditionally and you can't remove state without crossing Component boundaries, which makes this a different kind of fix. So while we are only suggesting "calculate in render" as a fix we should disallow the lint from throwing in this case IMO Test Plan: Added a fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35135). * __->__ facebook#35135 * facebook#35134
1 parent de97ef9 commit 257b033

File tree

4 files changed

+86
-1
lines changed

4 files changed

+86
-1
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,18 @@ function validateEffect(
690690
instr.value.args.length === 1 &&
691691
instr.value.args[0].kind === 'Identifier'
692692
) {
693+
const calleeMetadata = context.derivationCache.cache.get(
694+
instr.value.callee.identifier.id,
695+
);
696+
697+
/*
698+
* If the setState comes from a source other than local state skip
699+
* since the fix is not to calculate in render
700+
*/
701+
if (calleeMetadata?.typeOfValue != 'fromState') {
702+
continue;
703+
}
704+
693705
const argMetadata = context.derivationCache.cache.get(
694706
instr.value.args[0].identifier.id,
695707
);
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
6+
7+
function Component({setParentState, prop}) {
8+
useEffect(() => {
9+
setParentState(prop);
10+
}, [prop]);
11+
12+
return <div>{prop}</div>;
13+
}
14+
15+
```
16+
17+
## Code
18+
19+
```javascript
20+
import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
21+
22+
function Component(t0) {
23+
const $ = _c(7);
24+
const { setParentState, prop } = t0;
25+
let t1;
26+
if ($[0] !== prop || $[1] !== setParentState) {
27+
t1 = () => {
28+
setParentState(prop);
29+
};
30+
$[0] = prop;
31+
$[1] = setParentState;
32+
$[2] = t1;
33+
} else {
34+
t1 = $[2];
35+
}
36+
let t2;
37+
if ($[3] !== prop) {
38+
t2 = [prop];
39+
$[3] = prop;
40+
$[4] = t2;
41+
} else {
42+
t2 = $[4];
43+
}
44+
useEffect(t1, t2);
45+
let t3;
46+
if ($[5] !== prop) {
47+
t3 = <div>{prop}</div>;
48+
$[5] = prop;
49+
$[6] = t3;
50+
} else {
51+
t3 = $[6];
52+
}
53+
return t3;
54+
}
55+
56+
```
57+
58+
## Logs
59+
60+
```
61+
{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":105},"end":{"line":9,"column":1,"index":240},"filename":"from-props-setstate-in-effect-no-error.ts"},"fnName":"Component","memoSlots":7,"memoBlocks":3,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0}
62+
```
63+
64+
### Eval output
65+
(kind: exception) Fixture not implemented
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
2+
3+
function Component({setParentState, prop}) {
4+
useEffect(() => {
5+
setParentState(prop);
6+
}, [prop]);
7+
8+
return <div>{prop}</div>;
9+
}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ function Component(t0) {
6464
## Logs
6565

6666
```
67-
{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [second]\n\nData Flow Tree:\n└── second (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":14,"column":4,"index":443},"end":{"line":14,"column":8,"index":447},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null}
6867
{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":18,"column":1,"index":500},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0}
6968
```
7069

0 commit comments

Comments
 (0)