From 23830ea2a19bc14f5e7fa7198f9371130f0f8382 Mon Sep 17 00:00:00 2001 From: Timothy Yung Date: Mon, 12 Aug 2024 16:32:40 -0700 Subject: [PATCH 01/40] Unit Test for `findNodeHandle` Error Behavior (#30669) ## Summary As promised on https://github.com/facebook/react/pull/29627, this creates a unit test for the `findNodeHandle` error that prevents developers from calling it within render methods. ## How did you test this change? ``` $ yarn test ReactFabric-test.internal.js ``` --- .../__tests__/ReactFabric-test.internal.js | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js index d55be5efd24bfc..03f0cd0a6c0f3c 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -1306,6 +1306,40 @@ describe('ReactFabric', () => { ); }); + it('findNodeHandle errors when called from render', async () => { + class TestComponent extends React.Component { + render() { + ReactFabric.findNodeHandle(this); + return null; + } + } + await expect(async () => { + await act(() => { + ReactFabric.render(, 11, null, true); + }); + }).toErrorDev([ + 'TestComponent is accessing findNodeHandle inside its render(). ' + + 'render() should be a pure function of props and state. It should ' + + 'never access something that requires stale data from the previous ' + + 'render, such as refs. Move this logic to componentDidMount and ' + + 'componentDidUpdate instead.', + ]); + }); + + it("findNodeHandle doesn't error when called outside render", async () => { + class TestComponent extends React.Component { + render() { + return null; + } + componentDidMount() { + ReactFabric.findNodeHandle(this); + } + } + await act(() => { + ReactFabric.render(, 11, null, true); + }); + }); + it('should no-op if calling sendAccessibilityEvent on unmounted refs', async () => { const View = createReactNativeComponentClass('RCTView', () => ({ validAttributes: {foo: true}, From d48603a52564675ce02152fff245e38b6816da47 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Tue, 13 Aug 2024 11:31:44 -0400 Subject: [PATCH 02/40] Fix unstable_useContextWithBailout incorrect dispatcher assignment (#30673) Fixing a mistaken copy from another dispatcher property assignment --- packages/react-reconciler/src/ReactFiberHooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 3ba251b620713c..6e9e9b2c86ede9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -4839,7 +4839,7 @@ if (__DEV__) { }; } if (enableContextProfiling) { - (HooksDispatcherOnUpdateInDEV: Dispatcher).unstable_useContextWithBailout = + (InvalidNestedHooksDispatcherOnMountInDEV: Dispatcher).unstable_useContextWithBailout = function ( context: ReactContext, select: (T => Array) | null, From e865fe0722f889126b818d462a73026d7d0b6867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 13 Aug 2024 15:18:24 -0400 Subject: [PATCH 03/40] [DevTools] Unmount instance by walking the instance tree instead of the fiber tree (#30665) There was a comment that it's not safe to walk the unmounted fiber tree which I'm not sure is correct or not but we need to walk the instance tree to be able to clean up virtual instances anyway. We already walk the instance tree to clean up "remaining instances". This is also simpler because we don't need to special case Suspense boundaries. We simply clean up whatever branch we had before. The ultimate goal is to also walk the instance tree for updates so we don't need a fiber to instance map. --- .../src/backend/fiber/renderer.js | 119 ++++++------------ 1 file changed, 35 insertions(+), 84 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index d1bb0429e7c48f..3125bc060413a4 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -1148,8 +1148,9 @@ export function attach( // Recursively unmount all roots. hook.getFiberRoots(rendererID).forEach(root => { - currentRootID = getFiberInstanceThrows(root.current).id; - unmountFiberRecursively(root.current); + const rootInstance = getFiberInstanceThrows(root.current); + currentRootID = rootInstance.id; + unmountInstanceRecursively(rootInstance); flushPendingEvents(root); currentRootID = -1; }); @@ -2183,7 +2184,8 @@ export function attach( return fiberInstance; } - function recordUnmount(fiber: Fiber): null | FiberInstance { + function recordUnmount(fiberInstance: FiberInstance): void { + const fiber = fiberInstance.data; if (__DEBUG__) { debug('recordUnmount()', fiber, null); } @@ -2200,25 +2202,13 @@ export function attach( } } - const fiberInstance = getFiberInstanceUnsafe(fiber); - if (fiberInstance === null) { - // If we've never seen this Fiber, it might be inside of a legacy render Suspense fragment (so the store is not even aware of it). - // In that case we can just ignore it or it will cause errors later on. - // One example of this is a Lazy component that never resolves before being unmounted. - // - // This also might indicate a Fast Refresh force-remount scenario. - // - // TODO: This is fragile and can obscure actual bugs. - return null; - } - const id = fiberInstance.id; const isRoot = fiber.tag === HostRoot; if (isRoot) { // Roots must be removed only after all children have been removed. // So we track it separately. pendingUnmountedRootID = id; - } else if (!shouldFilterFiber(fiber)) { + } else { // To maintain child-first ordering, // we'll push it into one of these queues, // and later arrange them in the correct order. @@ -2232,7 +2222,6 @@ export function attach( idToRootMap.delete(id); idToTreeBaseDurationMap.delete(id); } - return fiberInstance; } // Running state of the remaining children from the previous version of this parent that @@ -2308,10 +2297,7 @@ export function attach( function unmountRemainingChildren() { let child = remainingReconcilingChildren; while (child !== null) { - if (child.kind === FIBER_INSTANCE) { - unmountFiberRecursively(child.data); - } - removeChild(child); + unmountInstanceRecursively(child); child = remainingReconcilingChildren; } } @@ -2434,69 +2420,33 @@ export function attach( // We use this to simulate unmounting for Suspense trees // when we switch from primary to fallback, or deleting a subtree. - function unmountFiberRecursively(fiber: Fiber) { + function unmountInstanceRecursively(instance: DevToolsInstance) { if (__DEBUG__) { - debug('unmountFiberRecursively()', fiber, null); + if (instance.kind === FIBER_INSTANCE) { + debug('unmountInstanceRecursively()', instance.data, null); + } } - let fiberInstance = null; - - const shouldIncludeInTree = !shouldFilterFiber(fiber); const stashedParent = reconcilingParent; const stashedPrevious = previouslyReconciledSibling; const stashedRemaining = remainingReconcilingChildren; - if (shouldIncludeInTree) { - fiberInstance = getFiberInstanceThrows(fiber); - // Push a new DevTools instance parent while reconciling this subtree. - reconcilingParent = fiberInstance; - previouslyReconciledSibling = null; - // Move all the children of this instance to the remaining set. - // We'll move them back one by one, and anything that remains is deleted. - remainingReconcilingChildren = fiberInstance.firstChild; - fiberInstance.firstChild = null; - } + // Push a new DevTools instance parent while reconciling this subtree. + reconcilingParent = instance; + previouslyReconciledSibling = null; + // Move all the children of this instance to the remaining set. + remainingReconcilingChildren = instance.firstChild; try { - // We might meet a nested Suspense on our way. - const isTimedOutSuspense = - fiber.tag === SuspenseComponent && fiber.memoizedState !== null; - - if (fiber.tag === HostHoistable) { - releaseHostResource(fiber, fiber.memoizedState); - } - - let child = fiber.child; - if (isTimedOutSuspense) { - // If it's showing fallback tree, let's traverse it instead. - const primaryChildFragment = fiber.child; - const fallbackChildFragment = primaryChildFragment - ? primaryChildFragment.sibling - : null; - // Skip over to the real Fiber child. - child = fallbackChildFragment ? fallbackChildFragment.child : null; - } - - unmountChildrenRecursively(child); + // Unmount the remaining set. + unmountRemainingChildren(); } finally { - if (shouldIncludeInTree) { - reconcilingParent = stashedParent; - previouslyReconciledSibling = stashedPrevious; - remainingReconcilingChildren = stashedRemaining; - } - } - if (fiberInstance !== null) { - recordUnmount(fiber); - removeChild(fiberInstance); + reconcilingParent = stashedParent; + previouslyReconciledSibling = stashedPrevious; + remainingReconcilingChildren = stashedRemaining; } - } - - function unmountChildrenRecursively(firstChild: null | Fiber) { - let child: null | Fiber = firstChild; - while (child !== null) { - // Record simulated unmounts children-first. - // We skip nodes without return because those are real unmounts. - unmountFiberRecursively(child); - child = child.sibling; + if (instance.kind === FIBER_INSTANCE) { + recordUnmount(instance); } + removeChild(instance); } function recordProfilingDurations(fiber: Fiber) { @@ -2843,7 +2793,8 @@ export function attach( } else if (!prevDidTimeout && nextDidTimeOut) { // Primary -> Fallback: // 1. Hide primary set - unmountChildrenRecursively(prevFiber.child); + // We simply don't re-add the fallback children and let + // unmountRemainingChildren() handle it. // 2. Mount fallback set const nextFiberChild = nextFiber.child; const nextFallbackChildSet = nextFiberChild @@ -3053,19 +3004,19 @@ export function attach( const current = root.current; const alternate = current.alternate; - const existingRoot = + let rootInstance = fiberToFiberInstanceMap.get(current) || (alternate && fiberToFiberInstanceMap.get(alternate)); - if (!existingRoot) { - const newRoot = createFiberInstance(current); - idToDevToolsInstanceMap.set(newRoot.id, newRoot); - fiberToFiberInstanceMap.set(current, newRoot); + if (!rootInstance) { + rootInstance = createFiberInstance(current); + idToDevToolsInstanceMap.set(rootInstance.id, rootInstance); + fiberToFiberInstanceMap.set(current, rootInstance); if (alternate) { - fiberToFiberInstanceMap.set(alternate, newRoot); + fiberToFiberInstanceMap.set(alternate, rootInstance); } - currentRootID = newRoot.id; + currentRootID = rootInstance.id; } else { - currentRootID = existingRoot.id; + currentRootID = rootInstance.id; } // Before the traversals, remember to start tracking @@ -3123,7 +3074,7 @@ export function attach( } else if (wasMounted && !isMounted) { // Unmount an existing root. removeRootPseudoKey(currentRootID); - unmountFiberRecursively(alternate); + unmountInstanceRecursively(rootInstance); } } else { // Mount a new root. From 082a690cc3ed6e27f62ed6e4ac655dec5c828708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 13 Aug 2024 15:18:34 -0400 Subject: [PATCH 04/40] [DevTools] Compute new reordered child set from the instance tree (#30668) This is already filtered and simply just all the ids in the linked list. Same principle as #30665. --- .../src/backend/fiber/renderer.js | 69 +++++-------------- 1 file changed, 16 insertions(+), 53 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 3125bc060413a4..0e80ef642a3acd 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -2516,24 +2516,28 @@ export function attach( } } - function recordResetChildren( - parentInstance: DevToolsInstance, - childSet: Fiber, - ) { + function recordResetChildren(parentInstance: DevToolsInstance) { if (__DEBUG__) { - debug('recordResetChildren()', childSet, parentInstance); + if ( + parentInstance.firstChild !== null && + parentInstance.firstChild.kind === FIBER_INSTANCE + ) { + debug( + 'recordResetChildren()', + parentInstance.firstChild.data, + parentInstance, + ); + } } // The frontend only really cares about the displayName, key, and children. // The first two don't really change, so we are only concerned with the order of children here. // This is trickier than a simple comparison though, since certain types of fibers are filtered. const nextChildren: Array = []; - // This is a naive implementation that shallowly recourses children. - // We might want to revisit this if it proves to be too inefficient. - let child: null | Fiber = childSet; + let child: null | DevToolsInstance = parentInstance.firstChild; while (child !== null) { - findReorderedChildrenRecursively(child, nextChildren); - child = child.sibling; + nextChildren.push(child.id); + child = child.nextSibling; } const numChildren = nextChildren.length; @@ -2549,38 +2553,6 @@ export function attach( } } - function findReorderedChildrenRecursively( - fiber: Fiber, - nextChildren: Array, - ) { - if (!shouldFilterFiber(fiber)) { - nextChildren.push(getFiberIDThrows(fiber)); - } else { - let child = fiber.child; - const isTimedOutSuspense = - fiber.tag === SuspenseComponent && fiber.memoizedState !== null; - if (isTimedOutSuspense) { - // Special case: if Suspense mounts in a timed-out state, - // get the fallback child from the inner fragment, - // and skip over the primary child. - const primaryChildFragment = fiber.child; - const fallbackChildFragment = primaryChildFragment - ? primaryChildFragment.sibling - : null; - const fallbackChild = fallbackChildFragment - ? fallbackChildFragment.child - : null; - if (fallbackChild !== null) { - child = fallbackChild; - } - } - while (child !== null) { - findReorderedChildrenRecursively(child, nextChildren); - child = child.sibling; - } - } - } - // Returns whether closest unfiltered fiber parent needs to reset its child list. function updateChildrenRecursively( nextFirstChild: null | Fiber, @@ -2865,17 +2837,8 @@ export function attach( // We need to crawl the subtree for closest non-filtered Fibers // so that we can display them in a flat children set. if (shouldIncludeInTree) { - // Normally, search for children from the rendered child. - let nextChildSet = nextFiber.child; - if (nextDidTimeOut) { - // Special case: timed-out Suspense renders the fallback set. - const nextFiberChild = nextFiber.child; - nextChildSet = nextFiberChild ? nextFiberChild.sibling : null; - } - if (nextChildSet != null) { - if (reconcilingParent !== null) { - recordResetChildren(reconcilingParent, nextChildSet); - } + if (reconcilingParent !== null) { + recordResetChildren(reconcilingParent); } // We've handled the child order change for this Fiber. // Since it's included, there's no need to invalidate parent child order. From a601d1da3647ed9a20d9f83b87c8019492f24215 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Tue, 13 Aug 2024 16:35:56 -0400 Subject: [PATCH 05/40] [compiler] Allow lets to be hoisted ghstack-source-id: 02f4698bd98705a855deb0d4bc30b9829afdddc0 Pull Request resolved: https://github.com/facebook/react/pull/30674 --- .../src/HIR/BuildHIR.ts | 16 ++++- .../src/HIR/HIR.ts | 8 ++- .../src/HIR/PrintHIR.ts | 3 + .../ReactiveScopes/CodegenReactiveFunction.ts | 23 +++++++ .../ReactiveScopes/PruneHoistedContexts.ts | 25 +++++-- .../ValidateLocalsNotReassignedAfterRender.ts | 10 +++ ...rences-variable-its-assigned-to.expect.md} | 2 +- ...on-references-variable-its-assigned-to.js} | 0 ...r.mutate-captured-arg-separately.expect.md | 31 --------- .../error.mutate-captured-arg-separately.js | 10 --- ...ences-later-variable-declaration.expect.md | 2 +- ...oisting-nested-let-declaration-2.expect.md | 62 ++++++++++++++++++ .../hoisting-nested-let-declaration-2.js | 17 +++++ .../hoisting-nested-let-declaration.expect.md | 65 +++++++++++++++++++ .../hoisting-nested-let-declaration.js | 21 ++++++ .../hoisting-simple-let-declaration.expect.md | 51 +++++++++++++++ .../hoisting-simple-let-declaration.js | 14 ++++ .../mutate-captured-arg-separately.expect.md | 56 ++++++++++++++++ .../mutate-captured-arg-separately.js | 16 +++++ 19 files changed, 381 insertions(+), 51 deletions(-) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-function-expression-references-variable-its-assigned-to.expect.md => error.function-expression-references-variable-its-assigned-to.expect.md} (59%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-function-expression-references-variable-its-assigned-to.js => error.function-expression-references-variable-its-assigned-to.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.js 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 b9bddff6a58b51..d5c7d9e3f41931 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -439,8 +439,11 @@ function lowerStatement( loc: id.parentPath.node.loc ?? GeneratedSource, }); continue; - } else if (binding.kind !== 'const' && binding.kind !== 'var') { - // Avoid double errors on var declarations, which we do not plan to support anyways + } else if ( + binding.kind !== 'const' && + binding.kind !== 'var' && + binding.kind !== 'let' + ) { builder.errors.push({ severity: ErrorSeverity.Todo, reason: 'Handle non-const declarations for hoisting', @@ -463,10 +466,17 @@ function lowerStatement( reactive: false, loc: id.node.loc ?? GeneratedSource, }; + const kind = + // Avoid double errors on var declarations, which we do not plan to support anyways + binding.kind === 'const' || binding.kind === 'var' + ? InstructionKind.HoistedConst + : binding.kind === 'let' + ? InstructionKind.HoistedLet + : assertExhaustive(binding.kind, 'Unexpected binding kind'); lowerValueToTemporary(builder, { kind: 'DeclareContext', lvalue: { - kind: InstructionKind.HoistedConst, + kind, place, }, loc: id.node.loc ?? GeneratedSource, diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index f249329e0c9f94..1e807d6d2a5d0e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -741,6 +741,9 @@ export enum InstructionKind { // hoisted const declarations HoistedConst = 'HoistedConst', + + // hoisted const declarations + HoistedLet = 'HoistedLet', } function _staticInvariantInstructionValueHasLocation( @@ -858,7 +861,10 @@ export type InstructionValue = | { kind: 'DeclareContext'; lvalue: { - kind: InstructionKind.Let | InstructionKind.HoistedConst; + kind: + | InstructionKind.Let + | InstructionKind.HoistedConst + | InstructionKind.HoistedLet; place: Place; }; loc: SourceLocation; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index fd17822af0a7ef..59f067787359f4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -760,6 +760,9 @@ export function printLValue(lval: LValue): string { case InstructionKind.HoistedConst: { return `HoistedConst ${lvalue}$`; } + case InstructionKind.HoistedLet: { + return `HoistedLet ${lvalue}$`; + } default: { assertExhaustive(lval.kind, `Unexpected lvalue kind \`${lval.kind}\``); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index bd3e97f23aba79..624a4b604d66f9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -994,6 +994,13 @@ function codegenTerminal( loc: iterableItem.loc, suggestions: null, }); + case InstructionKind.HoistedLet: + CompilerError.invariant(false, { + reason: 'Unexpected HoistedLet variable in for..in collection', + description: null, + loc: iterableItem.loc, + suggestions: null, + }); default: assertExhaustive( iterableItem.value.lvalue.kind, @@ -1089,6 +1096,13 @@ function codegenTerminal( loc: iterableItem.loc, suggestions: null, }); + case InstructionKind.HoistedLet: + CompilerError.invariant(false, { + reason: 'Unexpected HoistedLet variable in for..of collection', + description: null, + loc: iterableItem.loc, + suggestions: null, + }); default: assertExhaustive( iterableItem.value.lvalue.kind, @@ -1289,6 +1303,15 @@ function codegenInstructionNullable( case InstructionKind.Catch: { return t.emptyStatement(); } + case InstructionKind.HoistedLet: { + CompilerError.invariant(false, { + reason: + 'Expected HoistedLet to have been pruned in PruneHoistedContexts', + description: null, + loc: instr.loc, + suggestions: null, + }); + } case InstructionKind.HoistedConst: { CompilerError.invariant(false, { reason: diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts index 8608b32298503e..1df211afc3ae46 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts @@ -23,11 +23,11 @@ import { * original instruction kind. */ export function pruneHoistedContexts(fn: ReactiveFunction): void { - const hoistedIdentifiers: HoistedIdentifiers = new Set(); + const hoistedIdentifiers: HoistedIdentifiers = new Map(); visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers); } -type HoistedIdentifiers = Set; +type HoistedIdentifiers = Map; class Visitor extends ReactiveFunctionTransform { override transformInstruction( @@ -39,7 +39,21 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.kind === 'DeclareContext' && instruction.value.lvalue.kind === 'HoistedConst' ) { - state.add(instruction.value.lvalue.place.identifier.declarationId); + state.set( + instruction.value.lvalue.place.identifier.declarationId, + InstructionKind.Const, + ); + return {kind: 'remove'}; + } + + if ( + instruction.value.kind === 'DeclareContext' && + instruction.value.lvalue.kind === 'HoistedLet' + ) { + state.set( + instruction.value.lvalue.place.identifier.declarationId, + InstructionKind.Let, + ); return {kind: 'remove'}; } @@ -47,6 +61,9 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.kind === 'StoreContext' && state.has(instruction.value.lvalue.place.identifier.declarationId) ) { + const kind = state.get( + instruction.value.lvalue.place.identifier.declarationId, + )!; return { kind: 'replace', value: { @@ -57,7 +74,7 @@ class Visitor extends ReactiveFunctionTransform { ...instruction.value, lvalue: { ...instruction.value.lvalue, - kind: InstructionKind.Const, + kind, }, type: null, kind: 'StoreLocal', diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts index 2dab01f86e8384..0ea1814349f7f1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts @@ -130,6 +130,16 @@ function getContextReassignment( */ contextVariables.add(value.lvalue.place.identifier.id); } + const reassignment = reassigningFunctions.get( + value.value.identifier.id, + ); + if (reassignment !== undefined) { + reassigningFunctions.set( + value.lvalue.place.identifier.id, + reassignment, + ); + reassigningFunctions.set(lvalue.identifier.id, reassignment); + } break; } default: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-variable-its-assigned-to.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.function-expression-references-variable-its-assigned-to.expect.md similarity index 59% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-variable-its-assigned-to.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.function-expression-references-variable-its-assigned-to.expect.md index c06ecc085f8e73..76ac6d77a27741 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-variable-its-assigned-to.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.function-expression-references-variable-its-assigned-to.expect.md @@ -18,7 +18,7 @@ function Component() { 1 | function Component() { 2 | let callback = () => { > 3 | callback = null; - | ^^^^^^^^^^^^^^^ Todo: Handle non-const declarations for hoisting. variable "callback" declared with let (3:3) + | ^^^^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `callback` cannot be reassigned after render (3:3) 4 | }; 5 | return
; 6 | } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-variable-its-assigned-to.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.function-expression-references-variable-its-assigned-to.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-variable-its-assigned-to.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.function-expression-references-variable-its-assigned-to.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md deleted file mode 100644 index ecfbdf64952554..00000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.expect.md +++ /dev/null @@ -1,31 +0,0 @@ - -## Input - -```javascript -// Let's not support identifiers defined after use for now. -function component(a) { - let y = function () { - m(x); - }; - - let x = {a}; - m(x); - return y; -} - -``` - - -## Error - -``` - 2 | function component(a) { - 3 | let y = function () { -> 4 | m(x); - | ^^^^ Todo: Handle non-const declarations for hoisting. variable "x" declared with let (4:4) - 5 | }; - 6 | - 7 | let x = {a}; -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.js deleted file mode 100644 index 22ea3e823317dd..00000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutate-captured-arg-separately.js +++ /dev/null @@ -1,10 +0,0 @@ -// Let's not support identifiers defined after use for now. -function component(a) { - let y = function () { - m(x); - }; - - let x = {a}; - m(x); - return y; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-later-variable-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-later-variable-declaration.expect.md index 174139eaae9e30..d53d6b9ea4f33a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-later-variable-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-function-expression-references-later-variable-declaration.expect.md @@ -20,7 +20,7 @@ function Component() { 1 | function Component() { 2 | let callback = () => { > 3 | onClick = () => {}; - | ^^^^^^^^^^^^^^^^^^ Todo: Handle non-const declarations for hoisting. variable "onClick" declared with let (3:3) + | ^^^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `onClick` cannot be reassigned after render (3:3) 4 | }; 5 | let onClick; 6 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md new file mode 100644 index 00000000000000..fe0c6618f51adb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.expect.md @@ -0,0 +1,62 @@ + +## Input + +```javascript +function hoisting(cond) { + let items = []; + if (cond) { + let foo = () => { + items.push(bar()); + }; + let bar = () => true; + foo(); + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [true], + isComponent: false, +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function hoisting(cond) { + const $ = _c(2); + let items; + if ($[0] !== cond) { + items = []; + if (cond) { + const foo = () => { + items.push(bar()); + }; + + let bar = _temp; + foo(); + } + $[0] = cond; + $[1] = items; + } else { + items = $[1]; + } + return items; +} +function _temp() { + return true; +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [true], + isComponent: false, +}; + +``` + +### Eval output +(kind: ok) [true] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.js new file mode 100644 index 00000000000000..27b8a8fff60105 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration-2.js @@ -0,0 +1,17 @@ +function hoisting(cond) { + let items = []; + if (cond) { + let foo = () => { + items.push(bar()); + }; + let bar = () => true; + foo(); + } + return items; +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [true], + isComponent: false, +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md new file mode 100644 index 00000000000000..faf7a064d748f4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +function hoisting() { + let qux = () => { + let result; + { + result = foo(); + } + return result; + }; + let foo = () => { + return bar + baz; + }; + let bar = 3; + const baz = 2; + return qux(); // OK: called outside of TDZ +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [], + isComponent: false, +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function hoisting() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const qux = () => { + let result; + + result = foo(); + return result; + }; + + let foo = () => bar + baz; + + let bar = 3; + const baz = 2; + t0 = qux(); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [], + isComponent: false, +}; + +``` + +### Eval output +(kind: ok) 5 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.js new file mode 100644 index 00000000000000..c4c8f2aa4c2938 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-nested-let-declaration.js @@ -0,0 +1,21 @@ +function hoisting() { + let qux = () => { + let result; + { + result = foo(); + } + return result; + }; + let foo = () => { + return bar + baz; + }; + let bar = 3; + const baz = 2; + return qux(); // OK: called outside of TDZ +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [], + isComponent: false, +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md new file mode 100644 index 00000000000000..8974664b0515f5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +function hoisting() { + let foo = () => { + return bar + baz; + }; + let bar = 3; + let baz = 2; + return foo(); // OK: called outside of TDZ for bar/baz +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [], + isComponent: false, +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function hoisting() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const foo = () => bar + baz; + + let bar = 3; + let baz = 2; + t0 = foo(); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [], + isComponent: false, +}; + +``` + +### Eval output +(kind: ok) 5 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.js new file mode 100644 index 00000000000000..b9a02619f5c9aa --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-simple-let-declaration.js @@ -0,0 +1,14 @@ +function hoisting() { + let foo = () => { + return bar + baz; + }; + let bar = 3; + let baz = 2; + return foo(); // OK: called outside of TDZ for bar/baz +} + +export const FIXTURE_ENTRYPOINT = { + fn: hoisting, + params: [], + isComponent: false, +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md new file mode 100644 index 00000000000000..f3f21f8f6222b7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +function component(a) { + let y = function () { + m(x); + }; + + let x = {a}; + m(x); + return y; +} + +function m(x) {} + +export const FIXTURE_ENTRYPOINT = { + fn: component, + params: [{name: 'Jason'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function component(a) { + const $ = _c(2); + let y; + if ($[0] !== a) { + y = function () { + m(x); + }; + + let x = { a }; + m(x); + $[0] = a; + $[1] = y; + } else { + y = $[1]; + } + return y; +} + +function m(x) {} + +export const FIXTURE_ENTRYPOINT = { + fn: component, + params: [{ name: "Jason" }], +}; + +``` + +### Eval output +(kind: ok) "[[ function params=0 ]]" \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.js new file mode 100644 index 00000000000000..9985aea7f2fc50 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mutate-captured-arg-separately.js @@ -0,0 +1,16 @@ +function component(a) { + let y = function () { + m(x); + }; + + let x = {a}; + m(x); + return y; +} + +function m(x) {} + +export const FIXTURE_ENTRYPOINT = { + fn: component, + params: [{name: 'Jason'}], +}; From f6d1df6648a8912ea30550507a9d400802dcdff4 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 13 Aug 2024 13:42:10 -0700 Subject: [PATCH 06/40] [Flight] erroring after abort should not result in unhandled rejection (#30675) When I implemented the ability to abort synchronoulsy in flight I made it possible for erroring async server components to cause an unhandled rejection error. In the current implementation if you abort during the synchronous phase of a Function Component and then throw an error in the synchronous phase React will not attach any promise handlers because it short circuits the thenable treatment and throws an AbortSigil instead. This change updates the rendering logic to ignore the rejecting component. --- .../src/__tests__/ReactFlightDOM-test.js | 69 +++++++++++++++++++ .../react-server/src/ReactFlightServer.js | 31 +++++---- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index cd0bb994a7aa6f..77a2241734d3ef 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -2485,4 +2485,73 @@ describe('ReactFlightDOM', () => {
, ); }); + + it('can error synchronously after aborting without an unhandled rejection error', async () => { + function App() { + return ( +
+ loading...

}> + +
+
+ ); + } + + const abortRef = {current: null}; + + async function ComponentThatAborts() { + abortRef.current(); + throw new Error('boom'); + } + + const {writable: flightWritable, readable: flightReadable} = + getTestStream(); + + await serverAct(() => { + const {pipe, abort} = ReactServerDOMServer.renderToPipeableStream( + , + webpackMap, + ); + abortRef.current = abort; + pipe(flightWritable); + }); + + assertConsoleErrorDev([ + 'The render was aborted by the server without a reason.', + ]); + + const response = + ReactServerDOMClient.createFromReadableStream(flightReadable); + + const {writable: fizzWritable, readable: fizzReadable} = getTestStream(); + + function ClientApp() { + return use(response); + } + + const shellErrors = []; + await serverAct(async () => { + ReactDOMFizzServer.renderToPipeableStream( + React.createElement(ClientApp), + { + onShellError(error) { + shellErrors.push(error.message); + }, + }, + ).pipe(fizzWritable); + }); + assertConsoleErrorDev([ + 'The render was aborted by the server without a reason.', + ]); + + expect(shellErrors).toEqual([]); + + const container = document.createElement('div'); + await readInto(container, fizzReadable); + expect(getMeaningfulChildren(container)).toEqual( +
+

loading...

+
, + ); + }); }); diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index f680a3d97e1fc0..10ceee8075bd32 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -997,6 +997,8 @@ function callWithDebugContextInDEV( } } +const voidHandler = () => {}; + function renderFunctionComponent( request: Request, task: Task, @@ -1101,6 +1103,14 @@ function renderFunctionComponent( } if (request.status === ABORTING) { + if ( + typeof result === 'object' && + result !== null && + typeof result.then === 'function' && + !isClientReference(result) + ) { + result.then(voidHandler, voidHandler); + } // If we aborted during rendering we should interrupt the render but // we don't need to provide an error because the renderer will encode // the abort error as the reason. @@ -1120,18 +1130,15 @@ function renderFunctionComponent( // If the thenable resolves to an element, then it was in a static position, // the return value of a Server Component. That doesn't need further validation // of keys. The Server Component itself would have had a key. - thenable.then( - resolvedValue => { - if ( - typeof resolvedValue === 'object' && - resolvedValue !== null && - resolvedValue.$$typeof === REACT_ELEMENT_TYPE - ) { - resolvedValue._store.validated = 1; - } - }, - () => {}, - ); + thenable.then(resolvedValue => { + if ( + typeof resolvedValue === 'object' && + resolvedValue !== null && + resolvedValue.$$typeof === REACT_ELEMENT_TYPE + ) { + resolvedValue._store.validated = 1; + } + }, voidHandler); } if (thenable.status === 'fulfilled') { return thenable.value; From 8e60bacd08215bd23f0bf05dde407cd133885aa1 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Tue, 13 Aug 2024 16:57:45 -0400 Subject: [PATCH 07/40] [compiler] Reordering of logical expressions ghstack-source-id: ad484f97451c65a2642618bfb9d540d6a53a19a6 Pull Request resolved: https://github.com/facebook/react/pull/30678 --- .../src/HIR/BuildHIR.ts | 15 +++++++ .../compiler/logical-reorder.flow.expect.md | 39 +++++++++++++++++++ .../fixtures/compiler/logical-reorder.flow.js | 12 ++++++ 3 files changed, 66 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.js 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 d5c7d9e3f41931..8ed71b5e62a7ef 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -2847,6 +2847,21 @@ function isReorderableExpression( allowLocalIdentifiers, ); } + case 'LogicalExpression': { + const logical = expr as NodePath; + return ( + isReorderableExpression( + builder, + logical.get('left'), + allowLocalIdentifiers, + ) && + isReorderableExpression( + builder, + logical.get('right'), + allowLocalIdentifiers, + ) + ); + } case 'ConditionalExpression': { const conditional = expr as NodePath; return ( diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.expect.md new file mode 100644 index 00000000000000..f07193ea9102be --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +//@flow + +const foo = undefined; + +component C(...{scope = foo ?? null}: any) { + return scope; +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{scope: undefined}], +}; + +``` + +## Code + +```javascript +const foo = undefined; + +function C(t0) { + const { scope: t1 } = t0; + const scope = t1 === undefined ? (foo ?? null) : t1; + return scope; +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{ scope: undefined }], +}; + +``` + +### Eval output +(kind: ok) null \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.js new file mode 100644 index 00000000000000..48b7b1d04f5053 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/logical-reorder.flow.js @@ -0,0 +1,12 @@ +//@flow + +const foo = undefined; + +component C(...{scope = foo ?? null}: any) { + return scope; +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{scope: undefined}], +}; From 2a540194adde100c1af2b57b346e62eee760524c Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 13 Aug 2024 20:59:45 -0700 Subject: [PATCH 08/40] [Flight] do not emit error after abort (#30683) When synchronously aborting in a non-async Function Component if you throw after aborting the task would error rather than abort because React never observed the AbortSignal. Using a sigil to throw after aborting during render isn't effective b/c the user code itself could throw so insteead we just read the request status. This is ok b/c we don't expect any tasks to still be pending after the currently running task finishes. However I found one instance where that wasn't true related to serializing thenables which I've fixed so we may find other cases. If we do, though it's almost certainly a bug in our task bookkeeping so we should just fix it if it comes up. I also updated `abort` to not set the status to ABORTING unless the status was OPEN. we don't want to ever leave CLOSED or CLOSING status --- .../src/__tests__/ReactFlightDOM-test.js | 96 +++++++++++++++++++ .../react-server/src/ReactFlightServer.js | 23 +++-- 2 files changed, 110 insertions(+), 9 deletions(-) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js index 77a2241734d3ef..718ddf6c5716c0 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js @@ -2554,4 +2554,100 @@ describe('ReactFlightDOM', () => { , ); }); + + it('can error synchronously after aborting in a synchronous Component', async () => { + const rejectError = new Error('bam!'); + const rejectedPromise = Promise.reject(rejectError); + rejectedPromise.catch(() => {}); + rejectedPromise.status = 'rejected'; + rejectedPromise.reason = rejectError; + + const resolvedValue =

hello world

; + const resolvedPromise = Promise.resolve(resolvedValue); + resolvedPromise.status = 'fulfilled'; + resolvedPromise.value = resolvedValue; + + function App() { + return ( +
+ loading...

}> + +
+ loading too...

}> + {rejectedPromise} +
+ loading three...

}> + {resolvedPromise} +
+
+ ); + } + + const abortRef = {current: null}; + + // This test is specifically asserting that this works with Sync Server Component + function ComponentThatAborts() { + abortRef.current(); + throw new Error('boom'); + } + + const {writable: flightWritable, readable: flightReadable} = + getTestStream(); + + await serverAct(() => { + const {pipe, abort} = ReactServerDOMServer.renderToPipeableStream( + , + webpackMap, + { + onError(e) { + console.error(e); + }, + }, + ); + abortRef.current = abort; + pipe(flightWritable); + }); + + assertConsoleErrorDev([ + 'The render was aborted by the server without a reason.', + 'bam!', + ]); + + const response = + ReactServerDOMClient.createFromReadableStream(flightReadable); + + const {writable: fizzWritable, readable: fizzReadable} = getTestStream(); + + function ClientApp() { + return use(response); + } + + const shellErrors = []; + await serverAct(async () => { + ReactDOMFizzServer.renderToPipeableStream( + React.createElement(ClientApp), + { + onShellError(error) { + shellErrors.push(error.message); + }, + }, + ).pipe(fizzWritable); + }); + assertConsoleErrorDev([ + 'The render was aborted by the server without a reason.', + 'bam!', + ]); + + expect(shellErrors).toEqual([]); + + const container = document.createElement('div'); + await readInto(container, fizzReadable); + expect(getMeaningfulChildren(container)).toEqual( +
+

loading...

+

loading too...

+

hello world

+
, + ); + }); }); diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 10ceee8075bd32..6c9536d95acf76 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -382,8 +382,6 @@ export type Request = { didWarnForKey: null | WeakSet, }; -const AbortSigil = {}; - const { TaintRegistryObjects, TaintRegistryValues, @@ -594,6 +592,8 @@ function serializeThenable( const digest = logRecoverableError(request, x, null); emitErrorChunk(request, newTask.id, digest, x); } + newTask.status = ERRORED; + request.abortableTasks.delete(newTask); return newTask.id; } default: { @@ -650,10 +650,10 @@ function serializeThenable( logPostpone(request, postponeInstance.message, newTask); emitPostponeChunk(request, newTask.id, postponeInstance); } else { - newTask.status = ERRORED; const digest = logRecoverableError(request, reason, newTask); emitErrorChunk(request, newTask.id, digest, reason); } + newTask.status = ERRORED; request.abortableTasks.delete(newTask); enqueueFlush(request); }, @@ -1114,7 +1114,8 @@ function renderFunctionComponent( // If we aborted during rendering we should interrupt the render but // we don't need to provide an error because the renderer will encode // the abort error as the reason. - throw AbortSigil; + // eslint-disable-next-line no-throw-literal + throw null; } if ( @@ -1616,7 +1617,8 @@ function renderElement( // lazy initializers are user code and could abort during render // we don't wan to return any value resolved from the lazy initializer // if it aborts so we interrupt rendering here - throw AbortSigil; + // eslint-disable-next-line no-throw-literal + throw null; } return renderElement( request, @@ -2183,7 +2185,7 @@ function renderModel( } } - if (thrownValue === AbortSigil) { + if (request.status === ABORTING) { task.status = ABORTED; const errorId: number = (request.fatalError: any); if (wasReactNode) { @@ -2357,7 +2359,8 @@ function renderModelDestructive( // lazy initializers are user code and could abort during render // we don't wan to return any value resolved from the lazy initializer // if it aborts so we interrupt rendering here - throw AbortSigil; + // eslint-disable-next-line no-throw-literal + throw null; } if (__DEV__) { const debugInfo: ?ReactDebugInfo = lazy._debugInfo; @@ -3690,7 +3693,7 @@ function retryTask(request: Request, task: Task): void { } } - if (x === AbortSigil) { + if (request.status === ABORTING) { request.abortableTasks.delete(task); task.status = ABORTED; const errorId: number = (request.fatalError: any); @@ -3909,7 +3912,9 @@ export function stopFlowing(request: Request): void { // This is called to early terminate a request. It creates an error at all pending tasks. export function abort(request: Request, reason: mixed): void { try { - request.status = ABORTING; + if (request.status === OPEN) { + request.status = ABORTING; + } const abortableTasks = request.abortableTasks; // We have tasks to abort. We'll emit one error row and then emit a reference // to that row from every row that's still remaining. From fbfe08de6126f04777da805a493e819989151edf Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 14 Aug 2024 13:35:06 +0100 Subject: [PATCH 09/40] fix[react-devtools/InspectedElement]: fixed border stylings when some of the panels are not rendered (#30676) Alternative to https://github.com/facebook/react/pull/30667. Basically wrap every section in a `div` with the same class, and only apply `border-bottom` for every instance, except for the last child. We are paying some cost by having more divs, but thats more explicit. --- .../Components/InspectedElementContextTree.js | 2 +- .../InspectedElementErrorsAndWarningsTree.js | 2 +- .../Components/InspectedElementHooksTree.css | 7 +- .../Components/InspectedElementHooksTree.js | 4 +- .../Components/InspectedElementPropsTree.js | 4 +- .../InspectedElementSharedStyles.css | 7 -- .../InspectedElementSourcePanel.css | 5 - .../Components/InspectedElementSourcePanel.js | 2 +- .../Components/InspectedElementStateTree.js | 2 +- .../InspectedElementStyleXPlugin.js | 2 +- .../InspectedElementSuspenseToggle.js | 2 +- .../views/Components/InspectedElementView.css | 12 +- .../views/Components/InspectedElementView.js | 114 ++++++++++-------- 13 files changed, 80 insertions(+), 85 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js index 5e7276b1698a75..044a6d9b48d119 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js @@ -60,7 +60,7 @@ export default function InspectedElementContextTree({ return null; } else { return ( -
+
{hasLegacyContext ? 'legacy context' : 'context'} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js index 4e192d027e8dfa..48b81103cfea22 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js @@ -136,7 +136,7 @@ function Tree({ return null; } return ( -
+
{label}