From 7ab939bc55f51a736e9cc093d9a1e03cd551e823 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Fri, 12 Dec 2025 08:29:37 -0800 Subject: [PATCH] [compiler] Hoisting State Up draft Summary: Just a draft for now I'm testing a few things --- ...idateNoDerivedComputationsInEffects_exp.ts | 165 ++++++++++++++---- 1 file changed, 130 insertions(+), 35 deletions(-) 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 index bb59cf1d468..7cc6a8afe46 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -511,6 +511,13 @@ type TreeNode = { children: Array; }; +type DataFlowInfo = { + rootSources: string; + trees: Array; + propsArr: Array; + stateArr: Array; +}; + function buildTreeNode( sourceId: IdentifierId, context: ValidationContext, @@ -628,6 +635,51 @@ function renderTree( return result; } +/** + * Builds the data flow information including trees and source categorization + */ +function buildDataFlowInfo( + sourceIds: Set, + context: ValidationContext, +): DataFlowInfo { + const propsSet = new Set(); + const stateSet = new Set(); + + const rootNodesMap = new Map(); + for (const id of sourceIds) { + const nodes = buildTreeNode(id, context); + for (const node of nodes) { + if (!rootNodesMap.has(node.name)) { + rootNodesMap.set(node.name, node); + } + } + } + const rootNodes = Array.from(rootNodesMap.values()); + + const trees = rootNodes.map((node, index) => + renderTree(node, '', index === rootNodes.length - 1, propsSet, stateSet), + ); + + const propsArr = Array.from(propsSet); + const stateArr = Array.from(stateSet); + + let rootSources = ''; + if (propsArr.length > 0) { + rootSources += `Props: [${propsArr.join(', ')}]`; + } + if (stateArr.length > 0) { + if (rootSources) rootSources += '\n'; + rootSources += `State: [${stateArr.join(', ')}]`; + } + + return { + rootSources, + trees, + propsArr, + stateArr, + }; +} + function getFnLocalDeps( fn: FunctionExpression | undefined, ): Set | undefined { @@ -792,47 +844,16 @@ function validateEffect( effectSetStateUsages.get(rootSetStateCall)!.size === context.setStateUsages.get(rootSetStateCall)!.size - 1 ) { - const propsSet = new Set(); - const stateSet = new Set(); - - const rootNodesMap = new Map(); - for (const id of derivedSetStateCall.sourceIds) { - const nodes = buildTreeNode(id, context); - for (const node of nodes) { - if (!rootNodesMap.has(node.name)) { - rootNodesMap.set(node.name, node); - } - } - } - const rootNodes = Array.from(rootNodesMap.values()); - - const trees = rootNodes.map((node, index) => - renderTree( - node, - '', - index === rootNodes.length - 1, - propsSet, - stateSet, - ), - ); - for (const dep of derivedSetStateCall.sourceIds) { if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) { return; } } - const propsArr = Array.from(propsSet); - const stateArr = Array.from(stateSet); - - let rootSources = ''; - if (propsArr.length > 0) { - rootSources += `Props: [${propsArr.join(', ')}]`; - } - if (stateArr.length > 0) { - if (rootSources) rootSources += '\n'; - rootSources += `State: [${stateArr.join(', ')}]`; - } + const {rootSources, trees} = buildDataFlowInfo( + derivedSetStateCall.sourceIds, + context, + ); const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user @@ -857,6 +878,80 @@ See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-o message: 'This should be computed during render, not in an effect', }), ); + } else if ( + rootSetStateCall !== null && + effectSetStateUsages.has(rootSetStateCall) && + context.setStateUsages.has(rootSetStateCall) && + effectSetStateUsages.get(rootSetStateCall)!.size < + context.setStateUsages.get(rootSetStateCall)!.size + ) { + for (const dep of derivedSetStateCall.sourceIds) { + if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) { + return; + } + } + + const {rootSources, trees} = buildDataFlowInfo( + derivedSetStateCall.sourceIds, + context, + ); + + // Find setState calls outside the effect + const allSetStateUsages = context.setStateUsages.get(rootSetStateCall)!; + const effectUsages = effectSetStateUsages.get(rootSetStateCall)!; + const outsideEffectUsages: Array = []; + + for (const usage of allSetStateUsages) { + if (!effectUsages.has(usage)) { + outsideEffectUsages.push(usage); + } + } + + const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user + +This setState call is setting a derived value that depends on the following reactive sources: + +${rootSources} + +Data Flow Tree: +${trees.join('\n')} + +This state is also being set outside of the effect. Consider hoisting the state up to a parent component and making this a controlled component. + +See: https://react.dev/learn/sharing-state-between-components`; + + const diagnosticDetails: Array<{ + kind: 'error'; + loc: SourceLocation; + message: string; + }> = [ + { + kind: 'error', + loc: derivedSetStateCall.value.callee.loc, + message: 'setState in effect', + }, + ]; + + for (const usage of outsideEffectUsages) { + diagnosticDetails.push({ + kind: 'error', + loc: usage, + message: 'setState outside effect', + }); + } + + let diagnostic = CompilerDiagnostic.create({ + description: description, + category: ErrorCategory.EffectDerivationsOfState, + reason: + 'Consider hoisting state to parent and making this a controlled component', + }); + + for (const detail of diagnosticDetails) { + diagnostic = diagnostic.withDetails(detail); + } + + context.errors.pushDiagnostic(diagnostic); } } }