diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index 587ca333a9..fb4912d362 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -28,6 +28,7 @@ import { isUseStateType, GeneratedSource, } from '../HIR'; +import {printInstruction} from '../HIR/PrintHIR'; import { eachInstructionOperand, eachTerminalOperand, @@ -170,6 +171,22 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } } + const compilerError = generateCompilerError( + setStateCalls, + effectSetStates, + errors, + ); + + if (compilerError.hasErrors()) { + throw compilerError; + } +} + +function generateCompilerError( + setStateCalls: Map>, + effectSetStates: Map>, + errors: Array, +): CompilerError { const throwableErrors = new CompilerError(); for (const error of errors) { let compilerDiagnostic: CompilerDiagnostic | undefined = undefined; @@ -193,20 +210,9 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { effectSetStates.get(error.setStateName)?.length && error.type !== 'fromState' ) { - compilerDiagnostic = CompilerDiagnostic.create({ - description: `${error.description} Derived values 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.`, - category: `Local state shadows parent state.`, - severity: ErrorSeverity.InvalidReact, - }).withDetail({ - kind: 'error', - loc: error.loc, - message: detailMessage, - }); - } else { compilerDiagnostic = CompilerDiagnostic.create({ description: `${error.description} This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.`, - category: `Derive values in render, not effects.`, + category: `Local state shadows parent state.`, severity: ErrorSeverity.InvalidReact, }).withDetail({ kind: 'error', @@ -218,14 +224,11 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { if (setStateCallArray.length === 0) { continue; } - const otherCalls = setStateCalls.get(key); - if (otherCalls && otherCalls.length > 1) { - for (const place of otherCalls) { - if ( - !setStateCallArray.some( - existing => JSON.stringify(existing) === JSON.stringify(place), - ) - ) { + + const nonUseEffectSetStateCalls = setStateCalls.get(key); + if (nonUseEffectSetStateCalls) { + for (const place of nonUseEffectSetStateCalls) { + if (!setStateCallArray.includes(place)) { compilerDiagnostic.withDetail({ kind: 'error', loc: place.loc, @@ -236,6 +239,16 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } } } + } else { + compilerDiagnostic = CompilerDiagnostic.create({ + description: `${error.description} Derived values 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.`, + category: `Derive values in render, not effects.`, + severity: ErrorSeverity.InvalidReact, + }).withDetail({ + kind: 'error', + loc: error.loc, + message: detailMessage, + }); } if (compilerDiagnostic) { @@ -243,9 +256,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } } - if (throwableErrors.hasErrors()) { - throw throwableErrors; - } + return throwableErrors; } function joinValue( @@ -440,7 +451,7 @@ function validateEffect( } if (!isUsingDerivedDeps) { - // no effect prop/state derived deps were used in the body + // no prop/state derived deps were used in the body of the effect return; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md index 0f2ace7f38..45dbe7521a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md @@ -36,7 +36,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value both props and local state [prefix, name]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value both props and local state [prefix, name]. Derived values 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. error.bug-derived-state-from-mixed-deps.ts:9:4 7 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md new file mode 100644 index 0000000000..5399aaf978 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md @@ -0,0 +1,58 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useState, useEffect} from 'react'; + +function Component({props, number}) { + const nothing = 0; + const missDirection = number; + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + setDisplayValue(props.prefix + missDirection + nothing); + }, [props.prefix, missDirection, nothing]); + + return ( +
{ + setDisplayValue('clicked'); + }}> + {displayValue} +
+ ); +} + +``` + + +## Error + +``` +Found 1 error: + +Error: Local state shadows parent state. + +This setState() appears to derive a value from props [props, number]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. + +error.derived-state-from-shadowed-props.ts:10:4 + 8 | + 9 | useEffect(() => { +> 10 | setDisplayValue(props.prefix + missDirection + nothing); + | ^^^^^^^^^^^^^^^ This state value shadows a value passed as a prop. + 11 | }, [props.prefix, missDirection, nothing]); + 12 | + 13 | return ( + +error.derived-state-from-shadowed-props.ts:16:8 + 14 |
{ +> 16 | setDisplayValue('clicked'); + | ^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent + 17 | }}> + 18 | {displayValue} + 19 |
+``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.js new file mode 100644 index 0000000000..6d362ef45f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects +import {useState, useEffect} from 'react'; + +function Component({props, number}) { + const nothing = 0; + const missDirection = number; + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + setDisplayValue(props.prefix + missDirection + nothing); + }, [props.prefix, missDirection, nothing]); + + return ( +
{ + setDisplayValue('clicked'); + }}> + {displayValue} +
+ ); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-conditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-conditional.expect.md index 34ad5c7ea7..c7a0da24e1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-conditional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-conditional.expect.md @@ -34,7 +34,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value from props [value]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value from props [value]. Derived values 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. error.derived-state-with-conditional.ts:9:6 7 | useEffect(() => { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-side-effects.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-side-effects.expect.md index 7131a55eb3..f55588cfff 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-side-effects.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-side-effects.expect.md @@ -32,7 +32,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value from props [value]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value from props [value]. Derived values 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. error.derived-state-with-side-effects.ts:9:4 7 | useEffect(() => { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md index e451a3db44..dedc775d1e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md @@ -26,7 +26,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value local state [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value local state [firstName, lastName]. Derived values 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. error.invalid-derived-computation-in-effect.ts:9:4 7 | const [fullName, setFullName] = useState(''); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-computed.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-computed.expect.md index 7bce45a777..e3efe63b7b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-computed.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-computed.expect.md @@ -31,7 +31,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value from props [props]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value from props [props]. Derived values 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. error.invalid-derived-state-from-props-computed.ts:9:4 7 | useEffect(() => { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md index 2db707f70b..94e6f5c1a0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md @@ -30,7 +30,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value from props [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value from props [firstName, lastName]. Derived values 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. error.invalid-derived-state-from-props-destructured.ts:8:4 6 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md index dfdb33d550..f0fac25c4d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md @@ -30,7 +30,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value from props [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value from props [firstName, lastName]. Derived values 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. error.invalid-derived-state-from-props-in-effect.ts:8:4 6 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.expect.md index b827bd5ab6..5ecd370e01 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.expect.md @@ -28,7 +28,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value from props [input]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value from props [input]. Derived values 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. error.invalid-derived-state-from-props-with-default-value.ts:9:4 7 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md index eab7f10304..416402e4e7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md @@ -38,7 +38,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value local state [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. +This setState() appears to derive a value local state [firstName, lastName]. Derived values 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. error.invalid-derived-state-from-state-in-effect.ts:10:4 8 |