diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 1085d4c69e061..a83b22651e741 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -103,6 +103,7 @@ import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoF import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects'; import {inferMutationAliasingRanges} from '../Inference/InferMutationAliasingRanges'; import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects'; +import {validateNoDerivedComputationsInEffects_exp} from '../Validation/ValidateNoDerivedComputationsInEffects_exp'; import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions'; export type CompilerPipelineValue = @@ -275,6 +276,10 @@ function runWithEnvironment( validateNoDerivedComputationsInEffects(hir); } + if (env.config.validateNoDerivedComputationsInEffects_exp) { + validateNoDerivedComputationsInEffects_exp(hir); + } + if (env.config.validateNoSetStateInEffects) { env.logErrors(validateNoSetStateInEffects(hir, env)); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 57567f325fd9e..5712526a34e87 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -334,6 +334,12 @@ export const EnvironmentConfigSchema = z.object({ */ validateNoDerivedComputationsInEffects: z.boolean().default(false), + /** + * Experimental: Validates that effects are not used to calculate derived data which could instead be computed + * during render. Generates a custom error message for each type of violation. + */ + validateNoDerivedComputationsInEffects_exp: z.boolean().default(false), + /** * Validates against creating JSX within a try block and recommends using an error boundary * instead. diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts new file mode 100644 index 0000000000000..a755d0e2c65fb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -0,0 +1,504 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {CompilerDiagnostic, CompilerError, Effect} from '..'; +import {ErrorCategory} from '../CompilerError'; +import { + BlockId, + FunctionExpression, + HIRFunction, + IdentifierId, + isSetStateType, + isUseEffectHookType, + Place, + CallExpression, + Instruction, + isUseStateType, + BasicBlock, + isUseRefType, + GeneratedSource, + SourceLocation, +} from '../HIR'; +import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; +import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; +import {assertExhaustive} from '../Utils/utils'; + +type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsAndState'; + +type DerivationMetadata = { + typeOfValue: TypeOfValue; + place: Place; + sourcesIds: Set; +}; + +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}
;