diff --git a/ReactVersions.js b/ReactVersions.js index ba7b222bf0688..dd8c78afb1337 100644 --- a/ReactVersions.js +++ b/ReactVersions.js @@ -33,7 +33,7 @@ const canaryChannelLabel = 'canary'; const rcNumber = 0; const stablePackages = { - 'eslint-plugin-react-hooks': '7.0.0', + 'eslint-plugin-react-hooks': '7.1.0', 'jest-react': '0.18.0', react: ReactVersion, 'react-art': ReactVersion, diff --git a/compiler/apps/playground/components/Editor/ConfigEditor.tsx b/compiler/apps/playground/components/Editor/ConfigEditor.tsx index 18f904d225f0e..cf80b48dbf0da 100644 --- a/compiler/apps/playground/components/Editor/ConfigEditor.tsx +++ b/compiler/apps/playground/components/Editor/ConfigEditor.tsx @@ -14,6 +14,7 @@ import React, { unstable_ViewTransition as ViewTransition, unstable_addTransitionType as addTransitionType, startTransition, + Activity, } from 'react'; import {Resizable} from 're-resizable'; import {useStore, useStoreDispatch} from '../StoreContext'; @@ -34,12 +35,8 @@ export default function ConfigEditor({ const [isExpanded, setIsExpanded] = useState(false); return ( - // TODO: Use when it is compatible with Monaco: https://github.com/suren-atoyan/monaco-react/issues/753 <> -
+ { startTransition(() => { @@ -49,11 +46,8 @@ export default function ConfigEditor({ }} formattedAppliedConfig={formattedAppliedConfig} /> -
-
+ + { startTransition(() => { @@ -62,7 +56,7 @@ export default function ConfigEditor({ }); }} /> -
+
); } @@ -122,7 +116,8 @@ function ExpandedEditor({ return ( + enter={{[CONFIG_PANEL_TRANSITION]: 'slide-in', default: 'none'}} + exit={{[CONFIG_PANEL_TRANSITION]: 'slide-out', default: 'none'}}> ; +}; + +type ValidationContext = { + readonly functions: Map; + readonly errors: CompilerError; + readonly derivationCache: DerivationCache; + readonly effects: Set; + readonly setStateCache: Map>; + readonly effectSetStateCache: Map>; +}; + +class DerivationCache { + hasChanges: boolean = false; + cache: Map = new Map(); + + snapshot(): boolean { + const hasChanges = this.hasChanges; + this.hasChanges = false; + return hasChanges; + } + + addDerivationEntry( + derivedVar: Place, + sourcesIds: Set, + typeOfValue: TypeOfValue, + ): void { + let newValue: DerivationMetadata = { + place: derivedVar, + sourcesIds: new Set(), + typeOfValue: typeOfValue ?? 'ignored', + }; + + if (sourcesIds !== undefined) { + for (const id of sourcesIds) { + const sourcePlace = this.cache.get(id)?.place; + + if (sourcePlace === undefined) { + continue; + } + + /* + * If the identifier of the source is a promoted identifier, then + * we should set the target as the source. + */ + if ( + sourcePlace.identifier.name === null || + sourcePlace.identifier.name?.kind === 'promoted' + ) { + newValue.sourcesIds.add(derivedVar.identifier.id); + } else { + newValue.sourcesIds.add(sourcePlace.identifier.id); + } + } + } + + if (newValue.sourcesIds.size === 0) { + newValue.sourcesIds.add(derivedVar.identifier.id); + } + + const existingValue = this.cache.get(derivedVar.identifier.id); + if ( + existingValue === undefined || + !this.isDerivationEqual(existingValue, newValue) + ) { + this.cache.set(derivedVar.identifier.id, newValue); + this.hasChanges = true; + } + } + + private isDerivationEqual( + a: DerivationMetadata, + b: DerivationMetadata, + ): boolean { + if (a.typeOfValue !== b.typeOfValue) { + return false; + } + if (a.sourcesIds.size !== b.sourcesIds.size) { + return false; + } + for (const id of a.sourcesIds) { + if (!b.sourcesIds.has(id)) { + return false; + } + } + return true; + } +} + +/** + * Validates that useEffect is not used for derived computations which could/should + * be performed in render. + * + * See https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state + * + * Example: + * + * ``` + * // 🔴 Avoid: redundant state and unnecessary Effect + * const [fullName, setFullName] = useState(''); + * useEffect(() => { + * setFullName(firstName + ' ' + lastName); + * }, [firstName, lastName]); + * ``` + * + * Instead use: + * + * ``` + * // ✅ Good: calculated during rendering + * const fullName = firstName + ' ' + lastName; + * ``` + */ +export function validateNoDerivedComputationsInEffects_exp( + fn: HIRFunction, +): void { + const functions: Map = new Map(); + const derivationCache = new DerivationCache(); + const errors = new CompilerError(); + const effects: Set = new Set(); + + const setStateCache: Map> = new Map(); + const effectSetStateCache: Map< + string | undefined | null, + Array + > = new Map(); + + const context: ValidationContext = { + functions, + errors, + derivationCache, + effects, + setStateCache, + effectSetStateCache, + }; + + if (fn.fnType === 'Hook') { + for (const param of fn.params) { + if (param.kind === 'Identifier') { + context.derivationCache.cache.set(param.identifier.id, { + place: param, + sourcesIds: new Set([param.identifier.id]), + typeOfValue: 'fromProps', + }); + context.derivationCache.hasChanges = true; + } + } + } else if (fn.fnType === 'Component') { + const props = fn.params[0]; + if (props != null && props.kind === 'Identifier') { + context.derivationCache.cache.set(props.identifier.id, { + place: props, + sourcesIds: new Set([props.identifier.id]), + typeOfValue: 'fromProps', + }); + context.derivationCache.hasChanges = true; + } + } + + let isFirstPass = true; + do { + for (const block of fn.body.blocks.values()) { + recordPhiDerivations(block, context); + for (const instr of block.instructions) { + recordInstructionDerivations(instr, context, isFirstPass); + } + } + + isFirstPass = false; + } while (context.derivationCache.snapshot()); + + for (const effect of effects) { + validateEffect(effect, context); + } + + if (errors.hasAnyErrors()) { + throw errors; + } +} + +function recordPhiDerivations( + block: BasicBlock, + context: ValidationContext, +): void { + for (const phi of block.phis) { + let typeOfValue: TypeOfValue = 'ignored'; + let sourcesIds: Set = new Set(); + for (const operand of phi.operands.values()) { + const operandMetadata = context.derivationCache.cache.get( + operand.identifier.id, + ); + + if (operandMetadata === undefined) { + continue; + } + + typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue); + sourcesIds.add(operand.identifier.id); + } + + if (typeOfValue !== 'ignored') { + context.derivationCache.addDerivationEntry( + phi.place, + sourcesIds, + typeOfValue, + ); + } + } +} + +function joinValue( + lvalueType: TypeOfValue, + valueType: TypeOfValue, +): TypeOfValue { + if (lvalueType === 'ignored') return valueType; + if (valueType === 'ignored') return lvalueType; + if (lvalueType === valueType) return lvalueType; + return 'fromPropsAndState'; +} + +function recordInstructionDerivations( + instr: Instruction, + context: ValidationContext, + isFirstPass: boolean, +): void { + let typeOfValue: TypeOfValue = 'ignored'; + const sources: Set = new Set(); + const {lvalue, value} = instr; + if (value.kind === 'FunctionExpression') { + context.functions.set(lvalue.identifier.id, value); + for (const [, block] of value.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + recordInstructionDerivations(instr, context, isFirstPass); + } + } + } else if (value.kind === 'CallExpression' || value.kind === 'MethodCall') { + const callee = + value.kind === 'CallExpression' ? value.callee : value.property; + if ( + isUseEffectHookType(callee.identifier) && + value.args.length === 2 && + value.args[0].kind === 'Identifier' && + value.args[1].kind === 'Identifier' + ) { + const effectFunction = context.functions.get(value.args[0].identifier.id); + if (effectFunction != null) { + context.effects.add(effectFunction.loweredFunc.func); + } + } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) { + const stateValueSource = value.args[0]; + if (stateValueSource.kind === 'Identifier') { + sources.add(stateValueSource.identifier.id); + } + typeOfValue = joinValue(typeOfValue, 'fromState'); + } + } + + for (const operand of eachInstructionOperand(instr)) { + if ( + isSetStateType(operand.identifier) && + operand.loc !== GeneratedSource && + isFirstPass + ) { + if (context.setStateCache.has(operand.loc.identifierName)) { + context.setStateCache.get(operand.loc.identifierName)!.push(operand); + } else { + context.setStateCache.set(operand.loc.identifierName, [operand]); + } + } + + const operandMetadata = context.derivationCache.cache.get( + operand.identifier.id, + ); + + if (operandMetadata === undefined) { + continue; + } + + typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue); + for (const id of operandMetadata.sourcesIds) { + sources.add(id); + } + } + + if (typeOfValue === 'ignored') { + return; + } + + for (const lvalue of eachInstructionLValue(instr)) { + context.derivationCache.addDerivationEntry(lvalue, sources, typeOfValue); + } + + for (const operand of eachInstructionOperand(instr)) { + switch (operand.effect) { + case Effect.Capture: + case Effect.Store: + case Effect.ConditionallyMutate: + case Effect.ConditionallyMutateIterator: + case Effect.Mutate: { + if (isMutable(instr, operand)) { + context.derivationCache.addDerivationEntry( + operand, + sources, + typeOfValue, + ); + } + break; + } + case Effect.Freeze: + case Effect.Read: { + // no-op + break; + } + case Effect.Unknown: { + CompilerError.invariant(false, { + reason: 'Unexpected unknown effect', + description: null, + details: [ + { + kind: 'error', + loc: operand.loc, + message: 'Unexpected unknown effect', + }, + ], + }); + } + default: { + assertExhaustive( + operand.effect, + `Unexpected effect kind \`${operand.effect}\``, + ); + } + } + } +} + +function validateEffect( + effectFunction: HIRFunction, + context: ValidationContext, +): void { + const seenBlocks: Set = new Set(); + + const effectDerivedSetStateCalls: Array<{ + value: CallExpression; + loc: SourceLocation; + sourceIds: Set; + typeOfValue: TypeOfValue; + }> = []; + + const globals: Set = new Set(); + for (const block of effectFunction.body.blocks.values()) { + for (const pred of block.preds) { + if (!seenBlocks.has(pred)) { + // skip if block has a back edge + return; + } + } + + for (const instr of block.instructions) { + // Early return if any instruction is deriving a value from a ref + if (isUseRefType(instr.lvalue.identifier)) { + return; + } + + for (const operand of eachInstructionOperand(instr)) { + if ( + isSetStateType(operand.identifier) && + operand.loc !== GeneratedSource + ) { + if (context.effectSetStateCache.has(operand.loc.identifierName)) { + context.effectSetStateCache + .get(operand.loc.identifierName)! + .push(operand); + } else { + context.effectSetStateCache.set(operand.loc.identifierName, [ + operand, + ]); + } + } + } + + if ( + instr.value.kind === 'CallExpression' && + isSetStateType(instr.value.callee.identifier) && + instr.value.args.length === 1 && + instr.value.args[0].kind === 'Identifier' + ) { + const argMetadata = context.derivationCache.cache.get( + instr.value.args[0].identifier.id, + ); + + if (argMetadata !== undefined) { + effectDerivedSetStateCalls.push({ + value: instr.value, + loc: instr.value.callee.loc, + sourceIds: argMetadata.sourcesIds, + typeOfValue: argMetadata.typeOfValue, + }); + } + } else if (instr.value.kind === 'CallExpression') { + const calleeMetadata = context.derivationCache.cache.get( + instr.value.callee.identifier.id, + ); + + if ( + calleeMetadata !== undefined && + (calleeMetadata.typeOfValue === 'fromProps' || + calleeMetadata.typeOfValue === 'fromPropsAndState') + ) { + // If the callee is a prop we can't confidently say that it should be derived in render + return; + } + + if (globals.has(instr.value.callee.identifier.id)) { + // If the callee is a global we can't confidently say that it should be derived in render + return; + } + } else if (instr.value.kind === 'LoadGlobal') { + globals.add(instr.lvalue.identifier.id); + for (const operand of eachInstructionOperand(instr)) { + globals.add(operand.identifier.id); + } + } + } + seenBlocks.add(block.id); + } + + for (const derivedSetStateCall of effectDerivedSetStateCalls) { + if ( + derivedSetStateCall.loc !== GeneratedSource && + context.effectSetStateCache.has(derivedSetStateCall.loc.identifierName) && + context.setStateCache.has(derivedSetStateCall.loc.identifierName) && + context.effectSetStateCache.get(derivedSetStateCall.loc.identifierName)! + .length === + context.setStateCache.get(derivedSetStateCall.loc.identifierName)! + .length - + 1 + ) { + const derivedDepsStr = Array.from(derivedSetStateCall.sourceIds) + .map(sourceId => { + const sourceMetadata = context.derivationCache.cache.get(sourceId); + return sourceMetadata?.place.identifier.name?.value; + }) + .filter(Boolean) + .join(', '); + + let description; + + if (derivedSetStateCall.typeOfValue === 'fromProps') { + description = `From props: [${derivedDepsStr}]`; + } else if (derivedSetStateCall.typeOfValue === 'fromState') { + description = `From local state: [${derivedDepsStr}]`; + } else { + description = `From props and local state: [${derivedDepsStr}]`; + } + + context.errors.pushDiagnostic( + CompilerDiagnostic.create({ + 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`, + category: ErrorCategory.EffectDerivationsOfState, + reason: + 'You might not need an effect. Derive values in render, not effects.', + }).withDetails({ + kind: 'error', + loc: derivedSetStateCall.value.callee.loc, + message: 'This should be computed during render, not in an effect', + }), + ); + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md new file mode 100644 index 0000000000000..ef817a3ebf24b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md @@ -0,0 +1,84 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, [initialName]); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(6); + const { initialName } = t0; + const [name, setName] = useState(""); + let t1; + let t2; + if ($[0] !== initialName) { + t1 = () => { + setName(initialName); + }; + t2 = [initialName]; + $[0] = initialName; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 = (e) => setName(e.target.value); + $[3] = t3; + } else { + t3 = $[3]; + } + let t4; + if ($[4] !== name) { + t4 = ( +
+ +
+ ); + $[4] = name; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ initialName: "John" }], +}; + +``` + +### Eval output +(kind: ok)
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.js new file mode 100644 index 0000000000000..502402be51a3f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, [initialName]); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md new file mode 100644 index 0000000000000..2924de0da6132 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md @@ -0,0 +1,85 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function MockComponent({onSet}) { + return
onSet('clicked')}>Mock Component
; +} + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + }, [propValue]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function MockComponent(t0) { + const $ = _c(2); + const { onSet } = t0; + let t1; + if ($[0] !== onSet) { + t1 =
onSet("clicked")}>Mock Component
; + $[0] = onSet; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +function Component(t0) { + const $ = _c(4); + const { propValue } = t0; + const [, setValue] = useState(null); + let t1; + let t2; + if ($[0] !== propValue) { + t1 = () => { + setValue(propValue); + }; + t2 = [propValue]; + $[0] = propValue; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 = ; + $[3] = t3; + } else { + t3 = $[3]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ propValue: "test" }], +}; + +``` + +### Eval output +(kind: ok)
Mock Component
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.js new file mode 100644 index 0000000000000..d33af16ec5901 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.js @@ -0,0 +1,20 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function MockComponent({onSet}) { + return
onSet('clicked')}>Mock Component
; +} + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + }, [propValue]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md new file mode 100644 index 0000000000000..4d0b6663e325a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(''); + + const myRef = useRef(null); + + useEffect(() => { + setLocal(myRef.current + test); + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 'testString'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState, useRef } from "react"; + +export default function Component(t0) { + const $ = _c(5); + const { test } = t0; + const [local, setLocal] = useState(""); + + const myRef = useRef(null); + let t1; + let t2; + if ($[0] !== test) { + t1 = () => { + setLocal(myRef.current + test); + }; + t2 = [test]; + $[0] = test; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== local) { + t3 = <>{local}; + $[3] = local; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ test: "testString" }], +}; + +``` + +### Eval output +(kind: ok) nulltestString \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js new file mode 100644 index 0000000000000..6b24f73ac7400 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js @@ -0,0 +1,19 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(''); + + const myRef = useRef(null); + + useEffect(() => { + setLocal(myRef.current + test); + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 'testString'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md new file mode 100644 index 0000000000000..c83ea552a6a78 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md @@ -0,0 +1,75 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue, onChange}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + onChange(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test', onChange: () => {}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(7); + const { propValue, onChange } = t0; + const [value, setValue] = useState(null); + let t1; + if ($[0] !== onChange || $[1] !== propValue) { + t1 = () => { + setValue(propValue); + onChange(); + }; + $[0] = onChange; + $[1] = propValue; + $[2] = t1; + } else { + t1 = $[2]; + } + let t2; + if ($[3] !== propValue) { + t2 = [propValue]; + $[3] = propValue; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== value) { + t3 =
{value}
; + $[5] = value; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ propValue: "test", onChange: () => {} }], +}; + +``` + +### Eval output +(kind: ok)
test
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js new file mode 100644 index 0000000000000..512df7cb36ebd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js @@ -0,0 +1,17 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue, onChange}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + onChange(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test', onChange: () => {}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.expect.md new file mode 100644 index 0000000000000..e17f1e26f6c20 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.expect.md @@ -0,0 +1,70 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + globalCall(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(5); + const { propValue } = t0; + const [value, setValue] = useState(null); + let t1; + let t2; + if ($[0] !== propValue) { + t1 = () => { + setValue(propValue); + globalCall(); + }; + t2 = [propValue]; + $[0] = propValue; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== value) { + t3 =
{value}
; + $[3] = value; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ propValue: "test" }], +}; + +``` + +### Eval output +(kind: exception) globalCall is not defined \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.js new file mode 100644 index 0000000000000..4cded6dcc8ef3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.js @@ -0,0 +1,17 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + globalCall(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md new file mode 100644 index 0000000000000..1fa7f7d7950e6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +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. + +error.derived-state-conditionally-in-effect.ts:9:6 + 7 | useEffect(() => { + 8 | if (enabled) { +> 9 | setLocalValue(value); + | ^^^^^^^^^^^^^ This should be computed during render, not in an effect + 10 | } else { + 11 | setLocalValue('disabled'); + 12 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.js new file mode 100644 index 0000000000000..2ccd52500c773 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.js @@ -0,0 +1,21 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value, enabled}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + if (enabled) { + setLocalValue(value); + } else { + setLocalValue('disabled'); + } + }, [value, enabled]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test', enabled: true}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md new file mode 100644 index 0000000000000..f30235a064a94 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({input = 'empty'}) { + const [currInput, setCurrInput] = useState(input); + const localConst = 'local const'; + + useEffect(() => { + setCurrInput(input + localConst); + }, [input, localConst]); + + return
{currInput}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{input: 'test'}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +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. + +error.derived-state-from-default-props.ts:9:4 + 7 | + 8 | useEffect(() => { +> 9 | setCurrInput(input + localConst); + | ^^^^^^^^^^^^ This should be computed during render, not in an effect + 10 | }, [input, localConst]); + 11 | + 12 | return
{currInput}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.js new file mode 100644 index 0000000000000..1a0f5126e7a0b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({input = 'empty'}) { + const [currInput, setCurrInput] = useState(input); + const localConst = 'local const'; + + useEffect(() => { + setCurrInput(input + localConst); + }, [input, localConst]); + + return
{currInput}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{input: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md new file mode 100644 index 0000000000000..779ddafc40184 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp + +import {useEffect, useState} from 'react'; + +function Component({shouldChange}) { + const [count, setCount] = useState(0); + + useEffect(() => { + if (shouldChange) { + setCount(count + 1); + } + }, [count]); + + return
{count}
; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +Derived values (From local state: [count]) 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-from-local-state-in-effect.ts:10:6 + 8 | useEffect(() => { + 9 | if (shouldChange) { +> 10 | setCount(count + 1); + | ^^^^^^^^ This should be computed during render, not in an effect + 11 | } + 12 | }, [count]); + 13 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.js new file mode 100644 index 0000000000000..9568e4900296e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.js @@ -0,0 +1,15 @@ +// @validateNoDerivedComputationsInEffects_exp + +import {useEffect, useState} from 'react'; + +function Component({shouldChange}) { + const [count, setCount] = useState(0); + + useEffect(() => { + if (shouldChange) { + setCount(count + 1); + } + }, [count]); + + return
{count}
; +} diff --git a/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 b/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 new file mode 100644 index 0000000000000..7b27b556b3e98 --- /dev/null +++ b/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 @@ -0,0 +1,53 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({firstName}) { + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState('John'); + + const middleName = 'D.'; + + useEffect(() => { + setFullName(firstName + ' ' + middleName + ' ' + lastName); + }, [firstName, middleName, lastName]); + + return ( +
+ setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John'}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +Derived values (From props and local state: [firstName, lastName]) 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-from-prop-local-state-and-component-scope.ts:11:4 + 9 | + 10 | useEffect(() => { +> 11 | setFullName(firstName + ' ' + middleName + ' ' + lastName); + | ^^^^^^^^^^^ This should be computed during render, not in an effect + 12 | }, [firstName, middleName, lastName]); + 13 | + 14 | return ( +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.js new file mode 100644 index 0000000000000..3090ef00412d8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.js @@ -0,0 +1,25 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({firstName}) { + const [lastName, setLastName] = useState('Doe'); + const [fullName, setFullName] = useState('John'); + + const middleName = 'D.'; + + useEffect(() => { + setFullName(firstName + ' ' + middleName + ' ' + lastName); + }, [firstName, middleName, lastName]); + + return ( +
+ setLastName(e.target.value)} /> +
{fullName}
+
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{firstName: 'John'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md new file mode 100644 index 0000000000000..7fadae5667fdb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +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. + +error.derived-state-from-prop-with-side-effect.ts:8:4 + 6 | + 7 | useEffect(() => { +> 8 | setLocalValue(value); + | ^^^^^^^^^^^^^ This should be computed during render, not in an effect + 9 | document.title = `Value: ${value}`; + 10 | }, [value]); + 11 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.js new file mode 100644 index 0000000000000..88c66ce1ef650 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({value}) { + const [localValue, setLocalValue] = useState(''); + + useEffect(() => { + setLocalValue(value); + document.title = `Value: ${value}`; + }, [value]); + + return
{localValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md new file mode 100644 index 0000000000000..aec543fcbf48c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md @@ -0,0 +1,50 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + + function localFunction() { + console.log('local function'); + } + + useEffect(() => { + setValue(propValue); + localFunction(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +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. + +error.effect-contains-local-function-call.ts:12:4 + 10 | + 11 | useEffect(() => { +> 12 | setValue(propValue); + | ^^^^^^^^ This should be computed during render, not in an effect + 13 | localFunction(); + 14 | }, [propValue]); + 15 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js new file mode 100644 index 0000000000000..1efb3177e51e5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js @@ -0,0 +1,22 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({propValue}) { + const [value, setValue] = useState(null); + + function localFunction() { + console.log('local function'); + } + + useEffect(() => { + setValue(propValue); + localFunction(); + }, [propValue]); + + return
{value}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md new file mode 100644 index 0000000000000..f1f755adfa803 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md @@ -0,0 +1,48 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('Taylor'); + const lastName = 'Swift'; + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +Derived values (From 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. + +error.invalid-derived-computation-in-effect.ts:11:4 + 9 | const [fullName, setFullName] = useState(''); + 10 | useEffect(() => { +> 11 | setFullName(firstName + ' ' + lastName); + | ^^^^^^^^^^^ This should be computed during render, not in an effect + 12 | }, [firstName, lastName]); + 13 | + 14 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.js new file mode 100644 index 0000000000000..17779a5b4c576 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.js @@ -0,0 +1,20 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component() { + const [firstName, setFirstName] = useState('Taylor'); + const lastName = 'Swift'; + + // 🔴 Avoid: redundant state and unnecessary Effect + const [fullName, setFullName] = useState(''); + useEffect(() => { + setFullName(firstName + ' ' + lastName); + }, [firstName, lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md new file mode 100644 index 0000000000000..3a0788969397a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +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. + +error.invalid-derived-state-from-computed-props.ts:9:4 + 7 | useEffect(() => { + 8 | const computed = props.prefix + props.value + props.suffix; +> 9 | setDisplayValue(computed); + | ^^^^^^^^^^^^^^^ This should be computed during render, not in an effect + 10 | }, [props.prefix, props.value, props.suffix]); + 11 | + 12 | return
{displayValue}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.js new file mode 100644 index 0000000000000..24afa944fc566 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.js @@ -0,0 +1,18 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component(props) { + const [displayValue, setDisplayValue] = useState(''); + + useEffect(() => { + const computed = props.prefix + props.value + props.suffix; + setDisplayValue(computed); + }, [props.prefix, props.value, props.suffix]); + + return
{displayValue}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prefix: '[', value: 'test', suffix: ']'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md new file mode 100644 index 0000000000000..b28692c67b307 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md @@ -0,0 +1,47 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({props}) { + const [fullName, setFullName] = useState( + props.firstName + ' ' + props.lastName + ); + + useEffect(() => { + setFullName(props.firstName + ' ' + props.lastName); + }, [props.firstName, props.lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{props: {firstName: 'John', lastName: 'Doe'}}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: You might not need an effect. Derive values in render, not effects. + +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. + +error.invalid-derived-state-from-destructured-props.ts:10:4 + 8 | + 9 | useEffect(() => { +> 10 | setFullName(props.firstName + ' ' + props.lastName); + | ^^^^^^^^^^^ This should be computed during render, not in an effect + 11 | }, [props.firstName, props.lastName]); + 12 | + 13 | return
{fullName}
; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.js new file mode 100644 index 0000000000000..bdfb47a2c6aad --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.js @@ -0,0 +1,19 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +export default function Component({props}) { + const [fullName, setFullName] = useState( + props.firstName + ' ' + props.lastName + ); + + useEffect(() => { + setFullName(props.firstName + ' ' + props.lastName); + }, [props.firstName, props.lastName]); + + return
{fullName}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{props: {firstName: 'John', lastName: 'Doe'}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md new file mode 100644 index 0000000000000..365ee1fef4548 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md @@ -0,0 +1,82 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(0); + + const myRef = useRef(null); + + useEffect(() => { + if (myRef.current) { + setLocal(test); + } else { + setLocal(test + test); + } + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 4}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { useEffect, useState, useRef } from "react"; + +export default function Component(t0) { + const $ = _c(5); + const { test } = t0; + const [local, setLocal] = useState(0); + + const myRef = useRef(null); + let t1; + let t2; + if ($[0] !== test) { + t1 = () => { + if (myRef.current) { + setLocal(test); + } else { + setLocal(test + test); + } + }; + + t2 = [test]; + $[0] = test; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== local) { + t3 = <>{local}; + $[3] = local; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ test: 4 }], +}; + +``` + +### Eval output +(kind: ok) 8 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js new file mode 100644 index 0000000000000..ee59ccb78f037 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js @@ -0,0 +1,23 @@ +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState, useRef} from 'react'; + +export default function Component({test}) { + const [local, setLocal] = useState(0); + + const myRef = useRef(null); + + useEffect(() => { + if (myRef.current) { + setLocal(test); + } else { + setLocal(test + test); + } + }, [test]); + + return <>{local}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{test: 4}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md index d97a665ae698c..ccbcf68a576b3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md @@ -3,6 +3,8 @@ ```javascript // @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + function BadExample() { const [firstName, setFirstName] = useState('Taylor'); const [lastName, setLastName] = useState('Swift'); @@ -10,7 +12,7 @@ function BadExample() { // 🔴 Avoid: redundant state and unnecessary Effect const [fullName, setFullName] = useState(''); useEffect(() => { - setFullName(capitalize(firstName + ' ' + lastName)); + setFullName(firstName + ' ' + lastName); }, [firstName, lastName]); return
{fullName}
; @@ -26,14 +28,14 @@ Found 1 error: 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) -error.invalid-derived-computation-in-effect.ts:9:4 - 7 | const [fullName, setFullName] = useState(''); - 8 | useEffect(() => { -> 9 | setFullName(capitalize(firstName + ' ' + lastName)); +error.invalid-derived-computation-in-effect.ts:11:4 + 9 | const [fullName, setFullName] = useState(''); + 10 | useEffect(() => { +> 11 | setFullName(firstName + ' ' + lastName); | ^^^^^^^^^^^ 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) - 10 | }, [firstName, lastName]); - 11 | - 12 | return
{fullName}
; + 12 | }, [firstName, lastName]); + 13 | + 14 | return
{fullName}
; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js index d803d3c4a3a1f..0209b47ce39bb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.js @@ -1,4 +1,6 @@ // @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + function BadExample() { const [firstName, setFirstName] = useState('Taylor'); const [lastName, setLastName] = useState('Swift'); @@ -6,7 +8,7 @@ function BadExample() { // 🔴 Avoid: redundant state and unnecessary Effect const [fullName, setFullName] = useState(''); useEffect(() => { - setFullName(capitalize(firstName + ' ' + lastName)); + setFullName(firstName + ' ' + lastName); }, [firstName, lastName]); return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md index 8592ae65e4b81..250114fdfb391 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md @@ -60,29 +60,7 @@ This argument is a function which may reassign or mutate `cache` after render, w > 22 | // The original issue is that `cache` was not memoized together with the returned | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > 23 | // function. This was because neither appears to ever be mutated — the function - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 24 | // is known to mutate `cache` but the function isn't called. - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 25 | // - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 26 | // The fix is to detect cases like this — functions that are mutable but not called - - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 27 | // and ensure that their mutable captures are aliased together into the same scope. - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 28 | const cache = new WeakMap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 29 | return input => { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 30 | let output = cache.get(input); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 31 | if (output == null) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 32 | output = map(input); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 33 | cache.set(input, output); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 34 | } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + … > 35 | return output; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > 36 | }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-preserve-memo-deps-mixed-optional-nonoptional-property-chain.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-preserve-memo-deps-mixed-optional-nonoptional-property-chain.expect.md index e9772e67993f9..45e3d365a8d37 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-preserve-memo-deps-mixed-optional-nonoptional-property-chain.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-preserve-memo-deps-mixed-optional-nonoptional-property-chain.expect.md @@ -64,20 +64,7 @@ error.todo-preserve-memo-deps-mixed-optional-nonoptional-property-chain.ts:7:25 > 8 | return identity({ | ^^^^^^^^^^^^^^^^^^^^^ > 9 | callback: () => { - | ^^^^^^^^^^^^^^^^^^^^^ -> 10 | // This is a bug in our dependency inference: we stop capturing dependencies - | ^^^^^^^^^^^^^^^^^^^^^ -> 11 | // after x.a.b?.c. But what this dependency is telling us is that if `x.a.b` - | ^^^^^^^^^^^^^^^^^^^^^ -> 12 | // was non-nullish, then we can access `.c.d?.e`. Thus we should take the - | ^^^^^^^^^^^^^^^^^^^^^ -> 13 | // full property chain, exactly as-is with optionals/non-optionals, as a - | ^^^^^^^^^^^^^^^^^^^^^ -> 14 | // dependency - | ^^^^^^^^^^^^^^^^^^^^^ -> 15 | return identity(x.a.b?.c.d?.e); - | ^^^^^^^^^^^^^^^^^^^^^ -> 16 | }, + … | ^^^^^^^^^^^^^^^^^^^^^ > 17 | }); | ^^^^^^^^^^^^^^^^^^^^^ diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md index 38d10ee0d1bbc..00af7ec6ad598 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md @@ -46,18 +46,7 @@ error.todo-syntax.ts:11:2 > 12 | () => { | ^^^^^^^^^^^ > 13 | try { - | ^^^^^^^^^^^ -> 14 | console.log(prop1); - | ^^^^^^^^^^^ -> 15 | } finally { - | ^^^^^^^^^^^ -> 16 | console.log('exiting'); - | ^^^^^^^^^^^ -> 17 | } - | ^^^^^^^^^^^ -> 18 | }, - | ^^^^^^^^^^^ -> 19 | [prop1], + … | ^^^^^^^^^^^ > 20 | AUTODEPS | ^^^^^^^^^^^ diff --git a/packages/eslint-plugin-react-hooks/CHANGELOG.md b/packages/eslint-plugin-react-hooks/CHANGELOG.md index 0aba9e00561f7..9f88047193e72 100644 --- a/packages/eslint-plugin-react-hooks/CHANGELOG.md +++ b/packages/eslint-plugin-react-hooks/CHANGELOG.md @@ -1,3 +1,10 @@ +## 7.0.1 + +- Disallowed passing inline `useEffectEvent` values as JSX props to guard against accidental propagation. ([#34820](https://github.com/facebook/react/pull/34820) by [@jf-eirinha](https://github.com/jf-eirinha)) +- Switch to `export =` so eslint-plugin-react-hooks emits correct types for consumers in Node16 ESM projects. ([#34949](https://github.com/facebook/react/pull/34949) by [@karlhorky](https://github.com/karlhorky)) +- Tightened the typing of `configs.flat` so the `configs` export is always defined. ([#34950](https://github.com/facebook/react/pull/34950) by [@poteto](https://github.com/poteto)) +- Fix named import runtime errors. ([#34951](https://github.com/facebook/react/pull/34951), [#34953](https://github.com/facebook/react/pull/34953) by [@karlhorky](https://github.com/karlhorky)) + ## 7.0.0 This release slims down presets to just 2 configurations (`recommended` and `recommended-latest`), and all compiler rules are enabled by default.