Skip to content

Commit 7331a81

Browse files
committed
[compiler] Hoisting State Up draft
Summary: Just a draft for now I'm testing a few things
1 parent c0b7c0d commit 7331a81

File tree

1 file changed

+130
-35
lines changed

1 file changed

+130
-35
lines changed

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts

Lines changed: 130 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,13 @@ type TreeNode = {
511511
children: Array<TreeNode>;
512512
};
513513

514+
type DataFlowInfo = {
515+
rootSources: string;
516+
trees: Array<string>;
517+
propsArr: Array<string>;
518+
stateArr: Array<string>;
519+
};
520+
514521
function buildTreeNode(
515522
sourceId: IdentifierId,
516523
context: ValidationContext,
@@ -628,6 +635,51 @@ function renderTree(
628635
return result;
629636
}
630637

638+
/**
639+
* Builds the data flow information including trees and source categorization
640+
*/
641+
function buildDataFlowInfo(
642+
sourceIds: Set<IdentifierId>,
643+
context: ValidationContext,
644+
): DataFlowInfo {
645+
const propsSet = new Set<string>();
646+
const stateSet = new Set<string>();
647+
648+
const rootNodesMap = new Map<string, TreeNode>();
649+
for (const id of sourceIds) {
650+
const nodes = buildTreeNode(id, context);
651+
for (const node of nodes) {
652+
if (!rootNodesMap.has(node.name)) {
653+
rootNodesMap.set(node.name, node);
654+
}
655+
}
656+
}
657+
const rootNodes = Array.from(rootNodesMap.values());
658+
659+
const trees = rootNodes.map((node, index) =>
660+
renderTree(node, '', index === rootNodes.length - 1, propsSet, stateSet),
661+
);
662+
663+
const propsArr = Array.from(propsSet);
664+
const stateArr = Array.from(stateSet);
665+
666+
let rootSources = '';
667+
if (propsArr.length > 0) {
668+
rootSources += `Props: [${propsArr.join(', ')}]`;
669+
}
670+
if (stateArr.length > 0) {
671+
if (rootSources) rootSources += '\n';
672+
rootSources += `State: [${stateArr.join(', ')}]`;
673+
}
674+
675+
return {
676+
rootSources,
677+
trees,
678+
propsArr,
679+
stateArr,
680+
};
681+
}
682+
631683
function getFnLocalDeps(
632684
fn: FunctionExpression | undefined,
633685
): Set<IdentifierId> | undefined {
@@ -792,47 +844,16 @@ function validateEffect(
792844
effectSetStateUsages.get(rootSetStateCall)!.size ===
793845
context.setStateUsages.get(rootSetStateCall)!.size - 1
794846
) {
795-
const propsSet = new Set<string>();
796-
const stateSet = new Set<string>();
797-
798-
const rootNodesMap = new Map<string, TreeNode>();
799-
for (const id of derivedSetStateCall.sourceIds) {
800-
const nodes = buildTreeNode(id, context);
801-
for (const node of nodes) {
802-
if (!rootNodesMap.has(node.name)) {
803-
rootNodesMap.set(node.name, node);
804-
}
805-
}
806-
}
807-
const rootNodes = Array.from(rootNodesMap.values());
808-
809-
const trees = rootNodes.map((node, index) =>
810-
renderTree(
811-
node,
812-
'',
813-
index === rootNodes.length - 1,
814-
propsSet,
815-
stateSet,
816-
),
817-
);
818-
819847
for (const dep of derivedSetStateCall.sourceIds) {
820848
if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) {
821849
return;
822850
}
823851
}
824852

825-
const propsArr = Array.from(propsSet);
826-
const stateArr = Array.from(stateSet);
827-
828-
let rootSources = '';
829-
if (propsArr.length > 0) {
830-
rootSources += `Props: [${propsArr.join(', ')}]`;
831-
}
832-
if (stateArr.length > 0) {
833-
if (rootSources) rootSources += '\n';
834-
rootSources += `State: [${stateArr.join(', ')}]`;
835-
}
853+
const {rootSources, trees} = buildDataFlowInfo(
854+
derivedSetStateCall.sourceIds,
855+
context,
856+
);
836857

837858
const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
838859
@@ -857,6 +878,80 @@ See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-o
857878
message: 'This should be computed during render, not in an effect',
858879
}),
859880
);
881+
} else if (
882+
rootSetStateCall !== null &&
883+
effectSetStateUsages.has(rootSetStateCall) &&
884+
context.setStateUsages.has(rootSetStateCall) &&
885+
effectSetStateUsages.get(rootSetStateCall)!.size <
886+
context.setStateUsages.get(rootSetStateCall)!.size
887+
) {
888+
for (const dep of derivedSetStateCall.sourceIds) {
889+
if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) {
890+
return;
891+
}
892+
}
893+
894+
const {rootSources, trees} = buildDataFlowInfo(
895+
derivedSetStateCall.sourceIds,
896+
context,
897+
);
898+
899+
// Find setState calls outside the effect
900+
const allSetStateUsages = context.setStateUsages.get(rootSetStateCall)!;
901+
const effectUsages = effectSetStateUsages.get(rootSetStateCall)!;
902+
const outsideEffectUsages: Array<SourceLocation> = [];
903+
904+
for (const usage of allSetStateUsages) {
905+
if (!effectUsages.has(usage)) {
906+
outsideEffectUsages.push(usage);
907+
}
908+
}
909+
910+
const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
911+
912+
This setState call is setting a derived value that depends on the following reactive sources:
913+
914+
${rootSources}
915+
916+
Data Flow Tree:
917+
${trees.join('\n')}
918+
919+
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.
920+
921+
See: https://react.dev/learn/sharing-state-between-components`;
922+
923+
const diagnosticDetails: Array<{
924+
kind: 'error';
925+
loc: SourceLocation;
926+
message: string;
927+
}> = [
928+
{
929+
kind: 'error',
930+
loc: derivedSetStateCall.value.callee.loc,
931+
message: 'setState in effect',
932+
},
933+
];
934+
935+
for (const usage of outsideEffectUsages) {
936+
diagnosticDetails.push({
937+
kind: 'error',
938+
loc: usage,
939+
message: 'setState outside effect',
940+
});
941+
}
942+
943+
let diagnostic = CompilerDiagnostic.create({
944+
description: description,
945+
category: ErrorCategory.EffectDerivationsOfState,
946+
reason:
947+
'Consider hoisting state to parent and making this a controlled component',
948+
});
949+
950+
for (const detail of diagnosticDetails) {
951+
diagnostic = diagnostic.withDetails(detail);
952+
}
953+
954+
context.errors.pushDiagnostic(diagnostic);
860955
}
861956
}
862957
}

0 commit comments

Comments
 (0)