[Flight] Let Errored/Blocked Direct References Turn Nearest Element Lazy #29823

sebmarkbage · 2024-06-10T01:46:48Z

Stacked on #29807.

This lets the nearest Suspense/Error Boundary handle it even if that boundary is defined by the model itself.

It also ensures that when we have an error during serialization of properties, those can be associated with the nearest JSX element and since we have a stack/owner for that element we can use it to point to the source code of that line. We can't track the source of any nested arbitrary objects deeper inside since objects don’t track their stacks but close enough. Ideally we have the property path but we don’t have that right now. We have a partial in the message itself.

Note: The component name (Counter) is lost in the first message because we don't print it in the Task. We use "use client" instead because we expect the next stack frame to have the name. We also don't include it in the actual error message because the Server doesn't know the component name yet. Ideally Client References should be able to have a name. If the nearest is a Host Component then we do use the name though. However, it's not actually inside that Component that the error happens it's in App and that points to the right line number.

An interesting case is that if something that's actually going to be consumed by the props to a Suspense/Error Boundary or the Client Component that wraps them fails, then it can't be handled by the boundary. However, a counter intuitive case might be when that's on the children props. E.g. <ErrorBoundary>{clientReferenceOrInvalidSerialization}</ErrorBoundary>. This value can be inspected by the boundary so it's not safe to pass it so if it's errored it is not caught.

Implementation

The first insight is that this is best solved on the Client rather than in the Server because that way it also covers Client References that end up erroring.

The key insight is that while we don't have a true stack when using JSON.parse and therefore no begin/complete we can still infer these phases for Elements because the first child of an Element is always '$' which is also a leaf. In depth first that's our begin phase. When the Element itself completes, we have the complete phase. Anything in between is within the Element.

Using this idea I was able to refactor the blocking tracking mechanism to stash the blocked information on initializingHandler and then on the way up do we let whatever is nearest handle it - whether that's an Element or the root Chunk. It's kind of like an Algebraic Effect.

cc @unstubbable This is something you might want to deep dive into to find more edge cases. I'm sure I've missed something.

vercel · 2024-06-10T01:46:52Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 11, 2024 10:26pm

react-sizebot · 2024-06-10T01:51:04Z

The size diff is too large to display in a single comment. The CircleCI job contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against 977c078

unstubbable · 2024-06-10T08:58:48Z

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -4126,6 +4127,11 @@ function beginWork(
      }
      break;
    }
+    case Throw: {
+      // This represents a Component that threw in the reconciliation phase.
+      // So we'll rethrow here. This might be


This might be what?

unstubbable · 2024-06-10T16:48:35Z

packages/react-client/src/ReactFlightClient.js

  // We do this before parsing in case we try to initialize the same chunk
  // while parsing the model. Such as in a cyclic reference.
-  const cyclicChunk: CyclicChunk<T> = (chunk: any);
-  cyclicChunk.status = CYCLIC;
+  const cyclicChunk: BlockedChunk<T> = (chunk: any);


Should we rename this to blockedChunk now that the cyclic chunks are not a separate concept anymore?

Yea I was considering but decided to keep it. It's not a separate status but it's still cyclic at this point which is why we feel comfortable invoke the listeners later in this function - because nobody should be listening to it (not 100% true because a require() into a module could but not likely).

unstubbable · 2024-06-11T08:14:45Z

packages/react-client/src/__tests__/ReactFlight-test.js

-        '\n    in div' + '\n    in ErrorBoundary (at **)' + '\n    in App'
+      ? '\n    in Throw' +
+        '\n    in div' +
+        '\n    in ErrorBoundary (at **)' +


I know that this is not new in this PR, but I'm wondering why the error boundary is included in the stack unconditionally. Shouldn't this be gated on enableOwnerStacks and be omitted from the stack when the flag is enabled? It is handled like that in this test. Why not here?

The stack we're comparing here is coming from the componentStack field passed to Error Boundaries. This will remain the parent stack and available in both DEV and PROD.

unstubbable · 2024-06-11T10:48:39Z

packages/react-client/src/ReactFlightClient.js

+      if (
+        initializingHandler !== null &&
+        isArray(parentObject) &&
+        key === '0'


What is the use case for guarding with isArray(parentObject) && key === '0'? I feel like we should either omit this here, or also consider it for inferring whether we should return REACT_ELEMENT_TYPE. Or did you have a special case in mind that I can't think of right now where this check would only be relevant for the initializingHandler?

You can pass a symbol in arbitrary places in theory { mySymbol: REACT_ELEMENT_TYPE }. I mean it's really considered internal so you probably shouldn't. But the only place we special case its usage is here:

https://github.com/facebook/react/blob/main/packages/react-client/src/ReactFlightClient.js#L1116

So it needs to line up with that check to be a balanced push/pop.

However, I did notice that we don't actually check isArray() there so either we should add that check or remove the one here.

unstubbable · 2024-06-11T14:28:06Z

packages/react-client/src/ReactFlightClient.js

-      initializingChunk.then(freeze, freeze);
-    } else {
-      Object.freeze(element.props);
+      blockedChunk.then(freeze, freeze);


If I'm not misreading the curly braces I think we are now freezing the props of blocked models in prod. I assume this is unintentional?

Nice catch. Yea. This should be gated on __DEV__.

unstubbable · 2024-06-11T14:37:43Z

packages/react-client/src/ReactFlightClient.js

+    initializingHandler = handler.parent;
+    if (handler.errored) {
+      // Something errored inside this Element's props. We can turn this Element
+      // into a Lazy so that we can still render up until that Lazy is rendered.


Should there be a new unit test for this? It's still a bit unclear to me how this affects siblings.

This is effectively covered by the Throw cases that has the new component stacks. It is split into a synchronous and an asynchronous case where the error is already known vs becomes known though. Not sure if both are covered but meh.

eps1lon

Code looks fine though I don't know enough about the potential edge cases.

eps1lon · 2024-06-11T20:02:53Z

packages/react-reconciler/src/ReactChildFiber.js

          returnFiber,
          oldFiber,
          unwrapThenable(thenable),
          lanes,
-          debugInfo,
        );


I thought we need to pushDebugInfo where we used to mergeDebugInfo and don't need to pushDebugInfo when we just passed debugInfo as an argument. But here we're now pushing where we didn't used to merge. Is this a bug fix or change in behavior?

It's a bug fix. I missed this before. It should've merged.

eps1lon · 2024-06-11T20:12:02Z

packages/react-reconciler/src/getComponentNameFromFiber.js

+      break;
+    case Throw: {
+      if (__DEV__) {
+        // For an error in child position we use the of the inner most parent component.


Suggested change

// For an error in child position we use the of the inner most parent component.

// For an error in child position we use the name of the inner most parent component.

?

eps1lon · 2024-06-11T20:33:32Z

there's sebmarkbage#5 for some improved type coverage if that makes sense to you.

…he end

While JSON.parse doesn't let us inspect the begin/complete phase to push/po we can rely on the REACT_ELEMENT_TYPE marker being a leaf that's in the beginning of the element as the begin an the element itself is the complete phase. This lets us more deeply handle suspense or errors in case there's an Error or Suspense boundary in the current model. The tricky bit is to keep in mind cycles and deep references that might now be broken apart.

It's just a blocked chunk that eagerly calls its listeners.

…ped.

We feel comfortable turning any Element into Lazy since it serializes as Node. So if any error happens inside of the deserialization such as if a direct reference errored or a client reference failed to load we can scope it to that element. That way if any Error boundaries were on the stack inside the same model, they can handle the error. This also gives us better debug info for things like serialization errors because they now can get a stack trace pointing to the exact JSX.

sebmarkbage · 2024-06-11T23:12:36Z

@acdlite Took a look but deferred to @unstubbable's deeper review. I think I've addressed everything.

The issue reported in facebook#30172 was fixed with facebook#29823. This PR also added the test [`should resolve deduped objects that are themselves blocked`](https://github.com/facebook/react/blob/6d2a97a7113dfac2ad45067001b7e49a98718324/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js#L348-L393), which tests a similar scenario. However, this test would have also succeeded before applying the changes from facebook#29823. Therefore, I believe it makes sense to add this additional test, which does not succeed without facebook#29823, to prevent regressions.

The issue reported in #30172 was fixed with #29823. The PR also added the test [`should resolve deduped objects that are themselves blocked`](https://github.com/facebook/react/blob/6d2a97a7113dfac2ad45067001b7e49a98718324/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js#L348-L393), which tests a similar scenario. However, the existing test would have also succeeded before applying the changes from #29823. Therefore, I believe it makes sense to add an additional test `should resolve deduped objects in nested children of blocked models`, which does not succeed without #29823, to prevent regressions.

sebmarkbage requested review from gnoff and eps1lon June 10, 2024 01:46

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 10, 2024

vercel bot deployed to Preview June 10, 2024 01:49 View deployment

sebmarkbage force-pushed the lazifyelements branch from a8eb44b to c1bff89 Compare June 10, 2024 02:13

vercel bot deployed to Preview June 10, 2024 02:15 View deployment

unstubbable reviewed Jun 11, 2024

View reviewed changes

vercel bot deployed to Preview June 11, 2024 17:33 View deployment

sebmarkbage force-pushed the lazifyelements branch from 06c2d4c to 7adebac Compare June 11, 2024 19:09

vercel bot deployed to Preview June 11, 2024 19:12 View deployment

eps1lon mentioned this pull request Jun 11, 2024

Increase type coverage for ReactComponentInfo sebmarkbage/react#5

Closed

eps1lon reviewed Jun 11, 2024

View reviewed changes

sebmarkbage and others added 7 commits June 11, 2024 18:14

Refactor initializing chunk handling to attach the blocked chunk at t…

f75182a

…he end

We don't need the cyclic concept anymore

73cdb99

It's just a blocked chunk that eagerly calls its listeners.

Fix bug where references deep inside client elements couldn't be dedu…

1b24920

…ped.

Addressed comments

89bf5f9

More type-safety

b452d4d

sebmarkbage force-pushed the lazifyelements branch from 7adebac to b452d4d Compare June 11, 2024 22:15

sebmarkbage added 2 commits June 11, 2024 18:19

Add Omit helper

7d3d388

Update comment typo

977c078

vercel bot deployed to Preview June 11, 2024 22:26 View deployment

sebmarkbage merged commit fb57fc5 into facebook:main Jun 11, 2024
44 checks passed

eps1lon mentioned this pull request Jun 13, 2024

Update React from 6230622a1a-20240610 to dfd30974ab-20240613 vercel/next.js#66711

Merged

unstubbable mentioned this pull request Jul 2, 2024

[React 19] Cannot assign to readonly property #30172

Closed

unstubbable mentioned this pull request Jul 3, 2024

Add regression test for #30172 #30198

Merged

eps1lon mentioned this pull request Jul 10, 2024

[Not for merge] Update React from a26e3f403e to f3e09d6328 vercel/next.js#67629

Closed

This was referenced Jul 25, 2024

Update React from 6230622a1a-20240610 to 76002254-20240724 vercel/next.js#68149

Merged

Update React from 6230622a1a-20240610 to 3208e73e-20240730 vercel/next.js#68330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flight] Let Errored/Blocked Direct References Turn Nearest Element Lazy #29823

[Flight] Let Errored/Blocked Direct References Turn Nearest Element Lazy #29823

sebmarkbage commented Jun 10, 2024 •

edited

Loading

vercel bot commented Jun 10, 2024 •

edited

Loading

react-sizebot commented Jun 10, 2024 •

edited

Loading

unstubbable Jun 10, 2024

unstubbable Jun 10, 2024

sebmarkbage Jun 11, 2024

unstubbable Jun 11, 2024

sebmarkbage Jun 11, 2024

unstubbable Jun 11, 2024

sebmarkbage Jun 11, 2024

unstubbable Jun 11, 2024

sebmarkbage Jun 11, 2024

unstubbable Jun 11, 2024

sebmarkbage Jun 11, 2024

eps1lon left a comment

eps1lon Jun 11, 2024

sebmarkbage Jun 11, 2024

eps1lon Jun 11, 2024

eps1lon commented Jun 11, 2024

sebmarkbage commented Jun 11, 2024

	// For an error in child position we use the of the inner most parent component.
	// For an error in child position we use the name of the inner most parent component.

[Flight] Let Errored/Blocked Direct References Turn Nearest Element Lazy #29823

[Flight] Let Errored/Blocked Direct References Turn Nearest Element Lazy #29823

Conversation

sebmarkbage commented Jun 10, 2024 • edited Loading

Implementation

vercel bot commented Jun 10, 2024 • edited Loading

react-sizebot commented Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Jun 11, 2024

sebmarkbage commented Jun 11, 2024

sebmarkbage commented Jun 10, 2024 •

edited

Loading

vercel bot commented Jun 10, 2024 •

edited

Loading

react-sizebot commented Jun 10, 2024 •

edited

Loading