diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index 8f44594c0031..452aa0ce329d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -3268,6 +3268,21 @@ function isReorderableExpression( ) ); } + case 'NewExpression': { + const newExpr = expr as NodePath; + const callee = newExpr.get('callee'); + return ( + callee.isExpression() && + isReorderableExpression(builder, callee, allowLocalIdentifiers) && + newExpr + .get('arguments') + .every( + arg => + arg.isExpression() && + isReorderableExpression(builder, arg, allowLocalIdentifiers), + ) + ); + } default: { return false; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-new-expression-default-param.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-new-expression-default-param.expect.md new file mode 100644 index 000000000000..5f2f3619b88f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-new-expression-default-param.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +// @loggerTestOnly @validateNoSetStateInEffects @outputMode:"lint" +import {useEffect, useState} from 'react'; + +// Bug: NewExpression default param value should not prevent set-state-in-effect validation +function Component({value = new Number()}) { + const [state, setState] = useState(0); + useEffect(() => { + setState(s => s + 1); + }); + return state; +} + +``` + +## Code + +```javascript +// @loggerTestOnly @validateNoSetStateInEffects @outputMode:"lint" +import { useEffect, useState } from "react"; + +// Bug: NewExpression default param value should not prevent set-state-in-effect validation +function Component({ value = new Number() }) { + const [state, setState] = useState(0); + useEffect(() => { + setState((s) => s + 1); + }); + return state; +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"category":"EffectSetState","reason":"Calling setState synchronously within an effect can trigger cascading renders","description":"Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:\n* Update external systems with the latest state from React.\n* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\nCalling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect)","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":8,"column":4,"index":313},"end":{"line":8,"column":12,"index":321},"filename":"invalid-setState-in-useEffect-new-expression-default-param.ts","identifierName":"setState"},"message":"Avoid calling setState() directly within an effect"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":5,"column":0,"index":203},"end":{"line":11,"column":1,"index":358},"filename":"invalid-setState-in-useEffect-new-expression-default-param.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":1,"prunedMemoValues":0} +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-new-expression-default-param.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-new-expression-default-param.js new file mode 100644 index 000000000000..a239f743a6cc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-new-expression-default-param.js @@ -0,0 +1,11 @@ +// @loggerTestOnly @validateNoSetStateInEffects @outputMode:"lint" +import {useEffect, useState} from 'react'; + +// Bug: NewExpression default param value should not prevent set-state-in-effect validation +function Component({value = new Number()}) { + const [state, setState] = useState(0); + useEffect(() => { + setState(s => s + 1); + }); + return state; +} diff --git a/packages/react-server/src/ReactFlightReplyServer.js b/packages/react-server/src/ReactFlightReplyServer.js index 5f58e918f3e8..34a4942ce076 100644 --- a/packages/react-server/src/ReactFlightReplyServer.js +++ b/packages/react-server/src/ReactFlightReplyServer.js @@ -1123,8 +1123,9 @@ function createMap( if ((model as any).$$consumed === true) { throw new Error('Already initialized Map.'); } - const map = new Map(model); + // This needs to come first to prevent the model from being consumed again in case of a cyclic reference. (model as any).$$consumed = true; + const map = new Map(model); return map; } @@ -1135,8 +1136,9 @@ function createSet(response: Response, model: Array): Set { if ((model as any).$$consumed === true) { throw new Error('Already initialized Set.'); } - const set = new Set(model); + // This needs to come first to prevent the model from being consumed again in case of a cyclic reference. (model as any).$$consumed = true; + const set = new Set(model); return set; } @@ -1147,9 +1149,10 @@ function extractIterator(response: Response, model: Array): Iterator { if ((model as any).$$consumed === true) { throw new Error('Already initialized Iterator.'); } + // This needs to come first to prevent the model from being consumed again in case of a cyclic reference. + (model as any).$$consumed = true; // $FlowFixMe[incompatible-use]: This uses raw Symbols because we're extracting from a native array. const iterator = model[Symbol.iterator](); - (model as any).$$consumed = true; return iterator; } diff --git a/scripts/rollup/build.js b/scripts/rollup/build.js index 66b74b436349..087c2e6a43b9 100644 --- a/scripts/rollup/build.js +++ b/scripts/rollup/build.js @@ -453,7 +453,8 @@ function getPlugins( globalName, filename, moduleType, - bundle.wrapWithModuleBoundaries + bundle.wrapWithModuleBoundaries, + bundle.wrapWithNodeDevGuard ); }, }, diff --git a/scripts/rollup/bundles.js b/scripts/rollup/bundles.js index dbf6160a847e..7b9508b9e29c 100644 --- a/scripts/rollup/bundles.js +++ b/scripts/rollup/bundles.js @@ -1235,12 +1235,15 @@ const bundles = [ // currently required in order for the package to be copied over correctly. // So, it would be worth improving that flow. name: 'eslint-plugin-react-hooks', - bundleTypes: [NODE_DEV, NODE_PROD, FB_WWW_DEV, FB_WWW_PROD, CJS_DTS], + bundleTypes: [NODE_DEV, NODE_PROD, FB_WWW_DEV, CJS_DTS], moduleType: ISOMORPHIC, entry: 'eslint-plugin-react-hooks/src/index.ts', global: 'ESLintPluginReactHooks', minifyWithProdErrorCodes: false, wrapWithModuleBoundaries: false, + // This is a Node.js build tool (ESLint plugin), not a www runtime bundle. + // Use process.env.NODE_ENV guard instead of __DEV__ for the dev wrapper. + wrapWithNodeDevGuard: true, preferBuiltins: true, externals: [ '@babel/core', diff --git a/scripts/rollup/wrappers.js b/scripts/rollup/wrappers.js index 1773253cc965..52ae3b21252d 100644 --- a/scripts/rollup/wrappers.js +++ b/scripts/rollup/wrappers.js @@ -510,7 +510,8 @@ function wrapWithTopLevelDefinitions( globalName, filename, moduleType, - wrapWithModuleBoundaries + wrapWithModuleBoundaries, + wrapWithNodeDevGuard ) { if (wrapWithModuleBoundaries) { switch (bundleType) { @@ -553,8 +554,14 @@ function wrapWithTopLevelDefinitions( return wrapper(source, globalName, filename, moduleType); } + // Node.js build tools (e.g. ESLint plugins) use process.env.NODE_ENV instead + // of __DEV__ even when building for FB_WWW, since they run in Node.js where + // __DEV__ is not defined. + const effectiveBundleType = + wrapWithNodeDevGuard && bundleType === FB_WWW_DEV ? NODE_DEV : bundleType; + // All the other packages. - const wrapper = topLevelDefinitionWrappers[bundleType]; + const wrapper = topLevelDefinitionWrappers[effectiveBundleType]; if (typeof wrapper !== 'function') { throw new Error(`Unsupported build type: ${bundleType}.`); }