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 bcae209aa2..1b185cef91 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -42,7 +42,7 @@ type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState'; type DerivationMetadata = { typeOfValue: TypeOfValue; place: Place; - sources: Set; + sources: Array; }; type ErrorMetadata = { @@ -50,6 +50,7 @@ type ErrorMetadata = { description: string | undefined; loc: SourceLocation; setStateName: string | undefined | null; + derivedDepsNames: Array; }; /** @@ -80,6 +81,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const functions: Map = new Map(); const locals: Map = new Map(); const derivationCache: Map = new Map(); + const shadowingUseState: Map> = new Map(); const effectSetStates: Map< string | undefined | null, @@ -94,7 +96,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { if (param.kind === 'Identifier') { derivationCache.set(param.identifier.id, { place: param, - sources: new Set([param]), + sources: [param], typeOfValue: 'fromProps', }); } @@ -104,7 +106,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { if (props != null && props.kind === 'Identifier') { derivationCache.set(props.identifier.id, { place: props, - sources: new Set([props]), + sources: [props], typeOfValue: 'fromProps', }); } @@ -116,7 +118,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { for (const instr of block.instructions) { const {lvalue, value} = instr; - parseInstr(instr, derivationCache, setStateCalls); + parseInstr(instr, derivationCache, setStateCalls, shadowingUseState); if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); @@ -168,6 +170,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const compilerError = generateCompilerError( setStateCalls, effectSetStates, + shadowingUseState, errors, ); @@ -179,21 +182,12 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { function generateCompilerError( setStateCalls: Map>, effectSetStates: Map>, + shadowingUseState: Map>, errors: Array, ): CompilerError { const throwableErrors = new CompilerError(); for (const error of errors) { let compilerDiagnostic: CompilerDiagnostic | undefined = undefined; - let detailMessage = ''; - switch (error.type) { - case 'fromProps': - detailMessage = 'This state value shadows a value passed as a prop.'; - break; - case 'fromPropsOrState': - detailMessage = - 'This state value shadows a value passed as a prop or a value from state.'; - break; - } /* * If we use a setState from an invalid useEffect elsewhere then we probably have to @@ -205,15 +199,27 @@ function generateCompilerError( error.type !== 'fromState' ) { 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: `Local state shadows parent state.`, + description: `The setState within a useEffect is deriving from ${error.description}. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render`, + category: `You might not need an effect. Local state shadows parent state.`, severity: ErrorSeverity.InvalidReact, }).withDetail({ kind: 'error', loc: error.loc, - message: 'this setState synchronizes the state', + message: `this derives values from props ${error.type === 'fromPropsOrState' ? 'and local state ' : ''}to synchronize state`, }); + for (const derivedDep of error.derivedDepsNames) { + if (shadowingUseState.has(derivedDep)) { + for (const loc of shadowingUseState.get(derivedDep)!) { + compilerDiagnostic.withDetail({ + kind: 'error', + loc: loc, + message: `this useState shadows ${derivedDep}`, + }); + } + } + } + for (const [key, setStateCallArray] of effectSetStates) { if (setStateCallArray.length === 0) { continue; @@ -235,8 +241,8 @@ function generateCompilerError( } } 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.`, + description: `${error.description ? error.description.charAt(0).toUpperCase() + error.description.slice(1) : ''}. 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: `You might not need an effect. Derive values in render, not effects.`, severity: ErrorSeverity.InvalidReact, }).withDetail({ kind: 'error', @@ -271,7 +277,7 @@ function updateDerivationMetadata( ): void { let newValue: DerivationMetadata = { place: target, - sources: new Set(), + sources: [], typeOfValue: typeOfValue ?? 'ignored', }; @@ -286,9 +292,9 @@ function updateDerivationMetadata( place.identifier.name === null || place.identifier.name?.kind === 'promoted' ) { - newValue.sources.add(target); + newValue.sources.push(target); } else { - newValue.sources.add(place); + newValue.sources.push(place); } } } @@ -301,38 +307,19 @@ function parseInstr( instr: Instruction, derivationCache: Map, setStateCalls: Map>, + shadowingUseState: Map>, ): void { // Recursively parse function expressions + let typeOfValue: TypeOfValue = 'ignored'; + + let sources: Array = []; if (instr.value.kind === 'FunctionExpression') { for (const [, block] of instr.value.loweredFunc.func.body.blocks) { for (const instr of block.instructions) { - parseInstr(instr, derivationCache, setStateCalls); + parseInstr(instr, derivationCache, setStateCalls, shadowingUseState); } } - } - - let typeOfValue: TypeOfValue = 'ignored'; - - // Catch any useState hook calls - let sources: Array = []; - if ( - instr.value.kind === 'Destructure' && - instr.value.lvalue.pattern.kind === 'ArrayPattern' && - isUseStateType(instr.value.value.identifier) - ) { - typeOfValue = 'fromState'; - - const stateValueSource = instr.value.lvalue.pattern.items[0]; - if (stateValueSource.kind === 'Identifier') { - sources.push({ - place: stateValueSource, - typeOfValue: typeOfValue, - sources: new Set([stateValueSource]), - }); - } - } - - if ( + } else if ( instr.value.kind === 'CallExpression' && isSetStateType(instr.value.callee.identifier) && instr.value.args.length === 1 && @@ -348,6 +335,21 @@ function parseInstr( instr.value.callee, ]); } + } else if ( + (instr.value.kind === 'CallExpression' || + instr.value.kind === 'MethodCall') && + isUseStateType(instr.lvalue.identifier) + ) { + const stateValueSource = instr.value.args[0]; + if (stateValueSource.kind === 'Identifier') { + sources.push({ + place: stateValueSource, + typeOfValue: typeOfValue, + sources: [stateValueSource], + }); + } + + typeOfValue = joinValue(typeOfValue, 'fromState'); } for (const operand of eachInstructionOperand(instr)) { @@ -358,6 +360,27 @@ function parseInstr( typeOfValue = joinValue(typeOfValue, opSource.typeOfValue); sources.push(opSource); + + if ( + (instr.value.kind === 'CallExpression' || + instr.value.kind === 'MethodCall') && + opSource.typeOfValue === 'fromProps' && + isUseStateType(instr.lvalue.identifier) + ) { + opSource.sources.forEach(source => { + if (source.identifier.name !== null) { + if (shadowingUseState.has(source.identifier.name.value)) { + shadowingUseState + .get(source.identifier.name.value) + ?.push(instr.lvalue.loc); + } else { + shadowingUseState.set(source.identifier.name.value, [ + instr.lvalue.loc, + ]); + } + } + }); + } } if (typeOfValue !== 'ignored') { @@ -411,16 +434,26 @@ function parseBlockPhi( derivationCache: Map, ): void { for (const phi of block.phis) { + let typeOfValue: TypeOfValue = 'ignored'; + let sources: Array = []; for (const operand of phi.operands.values()) { - const phiSource = derivationCache.get(operand.identifier.id); - if (phiSource !== undefined) { - updateDerivationMetadata( - phi.place, - [phiSource], - phiSource?.typeOfValue, - derivationCache, - ); + const opSource = derivationCache.get(operand.identifier.id); + + if (opSource === undefined) { + continue; } + + typeOfValue = joinValue(typeOfValue, opSource?.typeOfValue ?? 'ignored'); + sources.push(opSource); + } + + if (typeOfValue !== 'ignored') { + updateDerivationMetadata( + phi.place, + sources, + typeOfValue, + derivationCache, + ); } } } @@ -524,7 +557,7 @@ function validateEffect( } for (const call of derivedSetStateCall) { - const placeNames = Array.from(call.derivedDep.sources) + const derivedDepsStr = Array.from(call.derivedDep.sources) .map(place => { return place.identifier.name?.value; }) @@ -534,19 +567,24 @@ function validateEffect( let errorDescription = ''; if (call.derivedDep.typeOfValue === 'fromProps') { - errorDescription = `props [${placeNames}].`; + errorDescription = `props [${derivedDepsStr}]`; } else if (call.derivedDep.typeOfValue === 'fromState') { - errorDescription = `local state [${placeNames}].`; + errorDescription = `local state [${derivedDepsStr}]`; } else { - errorDescription = `both props and local state [${placeNames}].`; + errorDescription = `both props and local state [${derivedDepsStr}]`; } errors.push({ type: call.derivedDep.typeOfValue, - description: `This setState() appears to derive a value from ${errorDescription}`, + description: `${errorDescription}`, loc: call.loc, setStateName: call.loc !== GeneratedSource ? call.loc.identifierName : undefined, + derivedDepsNames: Array.from(call.derivedDep.sources) + .map(place => { + return place.identifier.name?.value ?? ''; + }) + .filter(Boolean), }); } } 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 2588a014af..5255636da7 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 @@ -34,9 +34,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -This setState() appears to derive a value from 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. +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 index 66079d40bb..cd7c024fe9 100644 --- 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 @@ -32,19 +32,37 @@ function Component({props, number}) { ``` Found 1 error: -Error: Local state shadows parent state. +Error: You might not need an effect. 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. +The setState within a useEffect is deriving from props [props, number]. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render error.derived-state-from-shadowed-props.ts:10:4 8 | 9 | useEffect(() => { > 10 | setDisplayValue(props.prefix + missDirection + nothing); - | ^^^^^^^^^^^^^^^ this setState synchronizes the state + | ^^^^^^^^^^^^^^^ this derives values from props to synchronize state 11 | }, [props.prefix, missDirection, nothing]); 12 | 13 | return ( +error.derived-state-from-shadowed-props.ts:7:42 + 5 | const nothing = 0; + 6 | const missDirection = number; +> 7 | const [displayValue, setDisplayValue] = useState(props.prefix + missDirection + nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this useState shadows props + 8 | + 9 | useEffect(() => { + 10 | setDisplayValue(props.prefix + missDirection + nothing); + +error.derived-state-from-shadowed-props.ts:7:42 + 5 | const nothing = 0; + 6 | const missDirection = number; +> 7 | const [displayValue, setDisplayValue] = useState(props.prefix + missDirection + nothing); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this useState shadows number + 8 | + 9 | useEffect(() => { + 10 | setDisplayValue(props.prefix + missDirection + nothing); + error.derived-state-from-shadowed-props.ts:16:8 14 |
{ 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 0643af7722..5cf7a99730 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 @@ -32,9 +32,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -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. +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 0f25b76660..ba8d835199 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 @@ -30,9 +30,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -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. +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 bdf7a9b209..61ae320eec 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 @@ -24,9 +24,9 @@ function BadExample() { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -This setState() appears to derive a value from 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. +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 7773a2cc8d..daf74031d9 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 @@ -29,9 +29,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -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. +Props [props, 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 99b596c4ce..c5509ceae3 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 @@ -28,9 +28,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -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. +Props [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-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 88c722b8f6..9cb8a7427f 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 @@ -28,9 +28,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -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. +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 3af0c00ecc..8136511e6f 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 @@ -26,9 +26,9 @@ export default function InProductLobbyGeminiCard( ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -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. +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 5a029cb0cc..e7f8cd4584 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 @@ -36,9 +36,9 @@ export const FIXTURE_ENTRYPOINT = { ``` Found 1 error: -Error: Derive values in render, not effects. +Error: You might not need an effect. Derive values in render, not effects. -This setState() appears to derive a value from 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. +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 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.expect.md new file mode 100644 index 0000000000..f08a65dad2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.expect.md @@ -0,0 +1,61 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects + +function EndDate({startDate, endDate, onStartDateChange}) { + const [localStartDate, setLocalStartDate] = useState(startDate); + + useEffect(() => { + setLocalStartDate(startDate); + }, [startDate]); + + const onChange = (date) => { + setLocalStartDate(date); + onStartDateChange(date); + } + return +} + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Local state shadows parent state. + +The setState within a useEffect is deriving from props [startDate]. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render + +error.shadowed-props-with-onchange.ts:7:8 + 5 | + 6 | useEffect(() => { +> 7 | setLocalStartDate(startDate); + | ^^^^^^^^^^^^^^^^^ this derives values from props to synchronize state + 8 | }, [startDate]); + 9 | + 10 | const onChange = (date) => { + +error.shadowed-props-with-onchange.ts:4:47 + 2 | + 3 | function EndDate({startDate, endDate, onStartDateChange}) { +> 4 | const [localStartDate, setLocalStartDate] = useState(startDate); + | ^^^^^^^^^^^^^^^^^^^ this useState shadows startDate + 5 | + 6 | useEffect(() => { + 7 | setLocalStartDate(startDate); + +error.shadowed-props-with-onchange.ts:11:8 + 9 | + 10 | const onChange = (date) => { +> 11 | setLocalStartDate(date); + | ^^^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent + 12 | onStartDateChange(date); + 13 | } + 14 | return +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.js new file mode 100644 index 0000000000..52e74312e6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.js @@ -0,0 +1,15 @@ +// @validateNoDerivedComputationsInEffects + +function EndDate({startDate, endDate, onStartDateChange}) { + const [localStartDate, setLocalStartDate] = useState(startDate); + + useEffect(() => { + setLocalStartDate(startDate); + }, [startDate]); + + const onChange = (date) => { + setLocalStartDate(date); + onStartDateChange(date); + } + return +} diff --git a/compiler/yarn.lock b/compiler/yarn.lock index 696261cbf5..a2ae8a1acf 100644 --- a/compiler/yarn.lock +++ b/compiler/yarn.lock @@ -10494,16 +10494,7 @@ string-length@^4.0.1: char-regex "^1.0.2" strip-ansi "^6.0.0" -"string-width-cjs@npm:string-width@^4.2.0": - version "4.2.3" - resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz" - integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g== - dependencies: - emoji-regex "^8.0.0" - is-fullwidth-code-point "^3.0.0" - strip-ansi "^6.0.1" - -string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3: +"string-width-cjs@npm:string-width@^4.2.0", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3: version "4.2.3" resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz" integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g== @@ -10576,14 +10567,7 @@ string_decoder@~1.1.1: dependencies: safe-buffer "~5.1.0" -"strip-ansi-cjs@npm:strip-ansi@^6.0.1": - version "6.0.1" - resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz" - integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A== - dependencies: - ansi-regex "^5.0.1" - -strip-ansi@^6.0.0, strip-ansi@^6.0.1: +"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1: version "6.0.1" resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz" integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A== @@ -11360,7 +11344,7 @@ workerpool@^6.5.1: resolved "https://registry.npmjs.org/workerpool/-/workerpool-6.5.1.tgz" integrity sha512-Fs4dNYcsdpYSAfVxhnl1L5zTksjvOJxtC5hzMNl+1t9B8hTJTdKDyZ5ju7ztgPy+ft9tBFXoOlDNiOT9WUXZlA== -"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0": +"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0: version "7.0.0" resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz" integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q== @@ -11378,15 +11362,6 @@ wrap-ansi@^6.2.0: string-width "^4.1.0" strip-ansi "^6.0.0" -wrap-ansi@^7.0.0: - version "7.0.0" - resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz" - integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q== - dependencies: - ansi-styles "^4.0.0" - string-width "^4.1.0" - strip-ansi "^6.0.0" - wrap-ansi@^8.1.0: version "8.1.0" resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-8.1.0.tgz"