From 6160773f309b7fc734767649d47140d04fa1f568 Mon Sep 17 00:00:00 2001 From: Timothy Lau Date: Thu, 23 Oct 2025 23:13:18 +0800 Subject: [PATCH 1/5] [playground] Refactor ConfigEditor to use component (#34958) ## Summary This PR addresses a pending TODO comment left in https://github.com/facebook/react/pull/34499 https://github.com/facebook/react/blame/eb2f784e752ba690f032db4c3d87daac77a5a2aa/compiler/apps/playground/components/Editor/ConfigEditor.tsx#L37 This change removes the temporary workaround and replaces it with ``, as originally intended. ## How did you test this change? - Updated the component to use `` directly - Verified the editor renders correctly in both development and production builds. - The `` UI updates as expected. https://github.com/user-attachments/assets/ce976123-da59-4579-b063-b308a9167b21 --- .../components/Editor/ConfigEditor.tsx | 19 +++---- compiler/apps/playground/package.json | 14 ++--- compiler/apps/playground/styles/globals.css | 9 +++ compiler/apps/playground/yarn.lock | 57 ++++++++++--------- 4 files changed, 54 insertions(+), 45 deletions(-) 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'}}> Date: Thu, 23 Oct 2025 13:43:16 -0400 Subject: [PATCH 2/5] [eprh] Update changelog for 7.0.1 (#34964) Add changelog entry for 7.0.1 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34964). * #34965 * __->__ #34964 --- packages/eslint-plugin-react-hooks/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) 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. From c91783c1f2231ca5d99f5700f4c7fda4006a217e Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 23 Oct 2025 13:43:27 -0400 Subject: [PATCH 3/5] [eprh] Bump ReactVersions for next version (#34965) This was outdated from previously. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34965). * __->__ #34965 * #34964 --- ReactVersions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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, From 09056abde76c464f4632f322a0ac30cd3984cee6 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Thu, 23 Oct 2025 11:05:55 -0700 Subject: [PATCH 4/5] [Compiler] Improve error for calculate in render useEffect validation (#34580) Summary: Change error and update snapshots The error now mentions what values are causing the issue which should provide better context on how to fix the issue --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34580). * __->__ #34580 * #34579 * #34578 * #34577 * #34575 * #34574 --- .../src/Entrypoint/Pipeline.ts | 5 + .../src/HIR/Environment.ts | 6 + ...idateNoDerivedComputationsInEffects_exp.ts | 504 ++++++++++++++++++ ...ter-call-outside-effect-no-error.expect.md | 84 +++ ...rop-setter-call-outside-effect-no-error.js | 21 + ...ter-used-outside-effect-no-error.expect.md | 85 +++ ...rop-setter-used-outside-effect-no-error.js | 20 + ...tate-from-ref-and-state-no-error.expect.md | 73 +++ ...rived-state-from-ref-and-state-no-error.js | 19 + ...ains-prop-function-call-no-error.expect.md | 75 +++ ...ct-contains-prop-function-call-no-error.js | 17 + ...th-global-function-call-no-error.expect.md | 70 +++ ...fect-with-global-function-call-no-error.js | 17 + ...ed-state-conditionally-in-effect.expect.md | 49 ++ ...r.derived-state-conditionally-in-effect.js | 21 + ...derived-state-from-default-props.expect.md | 46 ++ .../error.derived-state-from-default-props.js | 18 + ...state-from-local-state-in-effect.expect.md | 43 ++ ...erived-state-from-local-state-in-effect.js | 15 + ...-local-state-and-component-scope.expect.md | 53 ++ ...om-prop-local-state-and-component-scope.js | 25 + ...state-from-prop-with-side-effect.expect.md | 46 ++ ...erived-state-from-prop-with-side-effect.js | 18 + ...ect-contains-local-function-call.expect.md | 50 ++ ...ror.effect-contains-local-function-call.js | 22 + ...id-derived-computation-in-effect.expect.md | 48 ++ ...r.invalid-derived-computation-in-effect.js | 20 + ...erived-state-from-computed-props.expect.md | 46 ++ ...valid-derived-state-from-computed-props.js | 18 + ...ed-state-from-destructured-props.expect.md | 47 ++ ...d-derived-state-from-destructured-props.js | 19 + ...f-conditional-in-effect-no-error.expect.md | 82 +++ .../ref-conditional-in-effect-no-error.js | 23 + ...id-derived-computation-in-effect.expect.md | 18 +- ...r.invalid-derived-computation-in-effect.js | 4 +- 35 files changed, 1718 insertions(+), 9 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts create mode 100644 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 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.js create mode 100644 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 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-ref-and-state-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-contains-prop-function-call-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-with-global-function-call-no-error.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.js create mode 100644 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 create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/ref-conditional-in-effect-no-error.js 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 bdbfb20a59ac1..efee8e080e3b8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -324,6 +324,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}
; From 408b38ef7304faf022d2a37110c57efce12c6bad Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Thu, 23 Oct 2025 11:30:28 -0700 Subject: [PATCH 5/5] [compiler] Improve display of errors on multi-line expressions (#34963) When a longer function or expression is identified as the source of an error, we currently print the entire expression in our error message. This is because we delegate to a Babel helper to print codeframes. Here, we add some checking and abbreviate the result if it spans too many lines. --- .../src/CompilerError.ts | 41 ++++++++++++++++++- ...es-memoizes-with-captures-values.expect.md | 24 +---------- ...ional-nonoptional-property-chain.expect.md | 15 +------ .../bailout-retry/error.todo-syntax.expect.md | 13 +----- 4 files changed, 43 insertions(+), 50 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index 3c131ea653ef3..18bcd7791d09f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -12,6 +12,28 @@ import {Err, Ok, Result} from './Utils/Result'; import {assertExhaustive} from './Utils/utils'; import invariant from 'invariant'; +// Number of context lines to display above the source of an error +const CODEFRAME_LINES_ABOVE = 2; +// Number of context lines to display below the source of an error +const CODEFRAME_LINES_BELOW = 3; +/* + * Max number of lines for the _source_ of an error, before we abbreviate + * the display of the source portion + */ +const CODEFRAME_MAX_LINES = 10; +/* + * When the error source exceeds the above threshold, how many lines of + * the source should be displayed? We show: + * - CODEFRAME_LINES_ABOVE context lines + * - CODEFRAME_ABBREVIATED_SOURCE_LINES of the error + * - '...' ellipsis + * - CODEFRAME_ABBREVIATED_SOURCE_LINES of the error + * - CODEFRAME_LINES_BELOW context lines + * + * This value must be at least 2 or else we'll cut off important parts of the error message + */ +const CODEFRAME_ABBREVIATED_SOURCE_LINES = 5; + export enum ErrorSeverity { /** * An actionable error that the developer can fix. For example, product code errors should be @@ -496,7 +518,7 @@ function printCodeFrame( loc: t.SourceLocation, message: string, ): string { - return codeFrameColumns( + const printed = codeFrameColumns( source, { start: { @@ -510,8 +532,25 @@ function printCodeFrame( }, { message, + linesAbove: CODEFRAME_LINES_ABOVE, + linesBelow: CODEFRAME_LINES_BELOW, }, ); + const lines = printed.split(/\r?\n/); + if (loc.end.line - loc.start.line < CODEFRAME_MAX_LINES) { + return printed; + } + const pipeIndex = lines[0].indexOf('|'); + return [ + ...lines.slice( + 0, + CODEFRAME_LINES_ABOVE + CODEFRAME_ABBREVIATED_SOURCE_LINES, + ), + ' '.repeat(pipeIndex) + '…', + ...lines.slice( + -(CODEFRAME_LINES_BELOW + CODEFRAME_ABBREVIATED_SOURCE_LINES), + ), + ].join('\n'); } function printErrorSummary(category: ErrorCategory, message: string): string { 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 | ^^^^^^^^^^^