Skip to content

Commit 49d604d

Browse files
committed
[Compiler] Improve error for calculate in render useEffect validation
Summary: Change error and update snapshots
1 parent 9b46b6c commit 49d604d

10 files changed

+69
-27
lines changed

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

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import {CompilerError, Effect} from '..';
8+
import {CompilerDiagnostic, CompilerError, Effect} from '..';
99
import {ErrorCategory} from '../CompilerError';
1010
import {
1111
BlockId,
@@ -305,6 +305,7 @@ function validateEffect(
305305
value: CallExpression;
306306
loc: SourceLocation;
307307
sourceIds: Set<IdentifierId>;
308+
typeOfValue: TypeOfValue;
308309
}> = [];
309310

310311
const globals: Set<IdentifierId> = new Set();
@@ -359,6 +360,7 @@ function validateEffect(
359360
value: instr.value,
360361
loc: instr.value.callee.loc,
361362
sourceIds: argMetadata.sourcesIds,
363+
typeOfValue: argMetadata.typeOfValue,
362364
});
363365
}
364366
} else if (instr.value.kind === 'CallExpression') {
@@ -398,14 +400,36 @@ function validateEffect(
398400
.length ===
399401
setStateCache.get(derivedSetStateCall.loc.identifierName)!.length
400402
) {
401-
errors.push({
402-
category: ErrorCategory.EffectDerivationsOfState,
403-
reason:
404-
'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)',
405-
description: null,
406-
loc: derivedSetStateCall.value.callee.loc,
407-
suggestions: null,
408-
});
403+
const derivedDepsStr = Array.from(derivedSetStateCall.sourceIds)
404+
.map(sourceId => {
405+
const sourceMetadata = derivationCache.get(sourceId);
406+
return sourceMetadata?.place.identifier.name?.value;
407+
})
408+
.filter(Boolean)
409+
.join(', ');
410+
411+
let description;
412+
413+
if (derivedSetStateCall.typeOfValue === 'fromProps') {
414+
description = `From props: [${derivedDepsStr}]`;
415+
} else if (derivedSetStateCall.typeOfValue === 'fromState') {
416+
description = `From local state: [${derivedDepsStr}]`;
417+
} else {
418+
description = `From props and local state: [${derivedDepsStr}]`;
419+
}
420+
421+
errors.pushDiagnostic(
422+
CompilerDiagnostic.create({
423+
description: `Derived values (${description}) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user`,
424+
category: ErrorCategory.EffectDerivationsOfState,
425+
reason:
426+
'You might not need an effect. Derive values in render, not effects.',
427+
}).withDetails({
428+
kind: 'error',
429+
loc: derivedSetStateCall.value.callee.loc,
430+
message: 'This should be computed during render, not in an effect',
431+
}),
432+
);
409433
}
410434
}
411435
}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,15 @@ export const FIXTURE_ENTRYPOINT = {
3232
```
3333
Found 1 error:
3434
35-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
35+
Error: You might not need an effect. Derive values in render, not effects.
36+
37+
Derived values (From props: [value]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3638
3739
error.derived-state-conditionally-in-effect.ts:9:6
3840
7 | useEffect(() => {
3941
8 | if (enabled) {
4042
> 9 | setLocalValue(value);
41-
| ^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
43+
| ^^^^^^^^^^^^^ This should be computed during render, not in an effect
4244
10 | } else {
4345
11 | setLocalValue('disabled');
4446
12 | }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,15 @@ export const FIXTURE_ENTRYPOINT = {
2929
```
3030
Found 1 error:
3131
32-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
32+
Error: You might not need an effect. Derive values in render, not effects.
33+
34+
Derived values (From props: [input]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3335
3436
error.derived-state-from-default-props.ts:9:4
3537
7 |
3638
8 | useEffect(() => {
3739
> 9 | setCurrInput(input + localConst);
38-
| ^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
40+
| ^^^^^^^^^^^^ This should be computed during render, not in an effect
3941
10 | }, [input, localConst]);
4042
11 |
4143
12 | return <div>{currInput}</div>;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,15 @@ export const FIXTURE_ENTRYPOINT = {
3636
```
3737
Found 1 error:
3838
39-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
39+
Error: You might not need an effect. Derive values in render, not effects.
40+
41+
Derived values (From props and local state: [firstName]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
4042
4143
error.derived-state-from-prop-local-state-and-component-scope.ts:11:4
4244
9 |
4345
10 | useEffect(() => {
4446
> 11 | setFullName(firstName + ' ' + middleName + ' ' + lastName);
45-
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
47+
| ^^^^^^^^^^^ This should be computed during render, not in an effect
4648
12 | }, [firstName, middleName, lastName]);
4749
13 |
4850
14 | return (

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,15 @@ export const FIXTURE_ENTRYPOINT = {
2929
```
3030
Found 1 error:
3131
32-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
32+
Error: You might not need an effect. Derive values in render, not effects.
33+
34+
Derived values (From props: [value]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3335
3436
error.derived-state-from-prop-with-side-effect.ts:8:4
3537
6 |
3638
7 | useEffect(() => {
3739
> 8 | setLocalValue(value);
38-
| ^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
40+
| ^^^^^^^^^^^^^ This should be computed during render, not in an effect
3941
9 | document.title = `Value: ${value}`;
4042
10 | }, [value]);
4143
11 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,15 @@ export const FIXTURE_ENTRYPOINT = {
3333
```
3434
Found 1 error:
3535
36-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
36+
Error: You might not need an effect. Derive values in render, not effects.
37+
38+
Derived values (From props: [propValue]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3739
3840
error.effect-contains-local-function-call.ts:12:4
3941
10 |
4042
11 | useEffect(() => {
4143
> 12 | setValue(propValue);
42-
| ^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
44+
| ^^^^^^^^ This should be computed during render, not in an effect
4345
13 | localFunction();
4446
14 | }, [propValue]);
4547
15 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,15 @@ export const FIXTURE_ENTRYPOINT = {
3131
```
3232
Found 1 error:
3333
34-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
34+
Error: You might not need an effect. Derive values in render, not effects.
35+
36+
Derived values (From local state: []) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3537
3638
error.invalid-derived-computation-in-effect.ts:11:4
3739
9 | const [fullName, setFullName] = useState('');
3840
10 | useEffect(() => {
3941
> 11 | setFullName(firstName + ' ' + lastName);
40-
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
42+
| ^^^^^^^^^^^ This should be computed during render, not in an effect
4143
12 | }, [firstName, lastName]);
4244
13 |
4345
14 | return <div>{fullName}</div>;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,15 @@ export const FIXTURE_ENTRYPOINT = {
2929
```
3030
Found 1 error:
3131
32-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
32+
Error: You might not need an effect. Derive values in render, not effects.
33+
34+
Derived values (From props: [props]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3335
3436
error.invalid-derived-state-from-computed-props.ts:9:4
3537
7 | useEffect(() => {
3638
8 | const computed = props.prefix + props.value + props.suffix;
3739
> 9 | setDisplayValue(computed);
38-
| ^^^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
40+
| ^^^^^^^^^^^^^^^ This should be computed during render, not in an effect
3941
10 | }, [props.prefix, props.value, props.suffix]);
4042
11 |
4143
12 | return <div>{displayValue}</div>;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,15 @@ export const FIXTURE_ENTRYPOINT = {
3030
```
3131
Found 1 error:
3232
33-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
33+
Error: You might not need an effect. Derive values in render, not effects.
34+
35+
Derived values (From props: [props]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3436
3537
error.invalid-derived-state-from-destructured-props.ts:10:4
3638
8 |
3739
9 | useEffect(() => {
3840
> 10 | setFullName(props.firstName + ' ' + props.lastName);
39-
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
41+
| ^^^^^^^^^^^ This should be computed during render, not in an effect
4042
11 | }, [props.firstName, props.lastName]);
4143
12 |
4244
13 | return <div>{fullName}</div>;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@ function BadExample() {
2626
```
2727
Found 1 error:
2828
29-
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
29+
Error: You might not need an effect. Derive values in render, not effects.
30+
31+
Derived values (From local state: []) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
3032
3133
error.invalid-derived-computation-in-effect.ts:11:4
3234
9 | const [fullName, setFullName] = useState('');
3335
10 | useEffect(() => {
3436
> 11 | setFullName(firstName + ' ' + lastName);
35-
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
37+
| ^^^^^^^^^^^ This should be computed during render, not in an effect
3638
12 | }, [firstName, lastName]);
3739
13 |
3840
14 | return <div>{fullName}</div>;

0 commit comments

Comments
 (0)