[Fizz] Add Component Stacks to onError and onPostpone when in dev mode or during prerenders in prod mode
#27761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
gnoff
force-pushed
the
prerender-stacks
branch
from
ea259c8
to
76f3394
Compare
gnoff
commented
Dec 8, 2023
Comment on lines
-2242
to
-2254
| } catch (x) { | ||
| if ( | ||
| typeof x === 'object' && | ||
| x !== null && | ||
| typeof x.then === 'function' | ||
| ) { | ||
| // this Lazy initializer is suspending. push a temporary frame onto the stack so it can be | ||
| // popped off in spawnNewSuspendedTask. This aligns stack behavior between Lazy in element position | ||
| // vs Component position. We do not want the frame for Errors so we exclusively do this in | ||
| // the wakeable branch | ||
| pushBuiltInComponentStackInDEV(task, 'Lazy'); | ||
| } | ||
| throw x; |
There was a problem hiding this comment.
I was originally motivated to remove this special case because I didn't want to have the extra try in prod. However I'm also not sure this was correct to begin with. The comment (which I wrote) indicates we are trying to have parity between stack behavior with element and Component position lazy's. However When I test the client behavior there is a seemingly buggy thing today where as an element if the lazy itself rejects you are told there is an error in component. However by removing this special case now elements and components both get a Lazy frame if the promise rejects and if the promise resolves this frame is removed from any future errors that arise deeper in the render. I think this makes sense but I wrote this comment for a reason and I no longer remember why. For now I'm going with this until we can clarify what the proper client behavior is since it should match
gnoff
commented
Dec 8, 2023
Comment on lines
-735
to
-742
| function pushFunctionComponentStackInDEV(task: Task, type: Function): void { | ||
| if (__DEV__) { | ||
| task.componentStack = { | ||
| tag: 1, | ||
| parent: task.componentStack, | ||
| type, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I refactored the way we track our stacks to be more efficient since we are running this in prod now. instead of pushing and popping we always stash and restore. To make this pattern clearer I renamed to get...() instead of push...() and moved the task mutation into the functions where these are used. This should inline better and eliminates a null check.
There was a problem hiding this comment.
Arguably this is more of a create prefix because you're not getting it from somewhere else.
sebmarkbage
approved these changes
Dec 16, 2023
gnoff
force-pushed
the
prerender-stacks
branch
from
76f3394
to
74ccffb
Compare
gnoff
force-pushed
the
prerender-stacks
branch
from
74ccffb
to
9325617
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Dec 16, 2023
… mode or during prerenders in prod mode (#27761) Historically React would produce component stacks for dev builds only. There is a cost to tracking component stacks and given the prod builds try to optimize runtime performance these stacks were left out. More recently React added production component stacks to Fiber in because it can be immensely helpful in tracking down hard to debug production issues. Fizz was not updated to have a similar behavior. With the advent of prerendering however stacks for production in Fizz are more relevant because prerendering is not really a dev-time task. If you want the ability to reason about errors or postpones that happen during a prerender having component stacks to interrogate is helpful and these component stacks need to be available in production otherwise you are really never going to see them. (it is possible that you could do dev-mode prerenders but we don't expect this to be a common dev mode workflow) To better support the prerender use case and to make error logging in Fizz more useful the following changes have been made 1. `onPostpone` now accepts a second `postponeInfo` argument which will contain a componentStack. Postpones always originate from a component render so the stack should be consistently available. The type however will indicate the stack is optional so we can remove them in the future if we decide the overhead is the wrong tradeoff in certain cases 2. `onError` now accepts a second `errorInfo` argument which may contain a componentStack. If an error originated from a component a stack will be included in the following cases. This change entails tracking the component hierarchy in prod builds now. While this isn't cost free it is implemented in a relatively lean manner. Deferring the most expensive work (reifying the stack) until we are actually in an error pathway. In the course of implementing this change a number of simplifications were made to the code which should make the stack tracking more resilient. We no longer use a module global to curry the stack up to some handler. This was delicate because you needed to always reset it properly. We now curry the stack on the task itself. Another change made was to track the component stack on SuspenseBoundary instances so that we can provide the stack when aborting suspense boundaries to help you determine which ones were affected by an abort. DiffTrain build for [63310df](63310df)
|
Just a heads up that this is causing a performance regression and we may need to back out or fix forward. |
neildhar
added a commit
to neildhar/react
that referenced
this pull request
Dec 19, 2023
…`onError` and `onPostpone` when in dev mode or during prerenders in prod mode (facebook#27761)" Summary: Backing out this change since it is the most likely culprit for a significant observed performance regression. Original commit changeset: bf4ae59939dc Original Phabricator Diffs: D52225293, D52290859 Reviewed By: josephsavona, fullstackhacker Differential Revision: D52307383
acdlite
added a commit
to vercel/next.js
that referenced
this pull request
Jan 8, 2024
### React upstream changes - facebook/react#27888 - facebook/react#27870 - facebook/react#27871 - facebook/react#27850 - facebook/react#27839 - facebook/react#27842 - facebook/react#27841 - facebook/react#27840 - facebook/react#27761 - facebook/react#27831 - facebook/react#27801 Closes NEXT-2012
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
… mode or during prerenders in prod mode (facebook#27761) Historically React would produce component stacks for dev builds only. There is a cost to tracking component stacks and given the prod builds try to optimize runtime performance these stacks were left out. More recently React added production component stacks to Fiber in because it can be immensely helpful in tracking down hard to debug production issues. Fizz was not updated to have a similar behavior. With the advent of prerendering however stacks for production in Fizz are more relevant because prerendering is not really a dev-time task. If you want the ability to reason about errors or postpones that happen during a prerender having component stacks to interrogate is helpful and these component stacks need to be available in production otherwise you are really never going to see them. (it is possible that you could do dev-mode prerenders but we don't expect this to be a common dev mode workflow) To better support the prerender use case and to make error logging in Fizz more useful the following changes have been made 1. `onPostpone` now accepts a second `postponeInfo` argument which will contain a componentStack. Postpones always originate from a component render so the stack should be consistently available. The type however will indicate the stack is optional so we can remove them in the future if we decide the overhead is the wrong tradeoff in certain cases 2. `onError` now accepts a second `errorInfo` argument which may contain a componentStack. If an error originated from a component a stack will be included in the following cases. This change entails tracking the component hierarchy in prod builds now. While this isn't cost free it is implemented in a relatively lean manner. Deferring the most expensive work (reifying the stack) until we are actually in an error pathway. In the course of implementing this change a number of simplifications were made to the code which should make the stack tracking more resilient. We no longer use a module global to curry the stack up to some handler. This was delicate because you needed to always reset it properly. We now curry the stack on the task itself. Another change made was to track the component stack on SuspenseBoundary instances so that we can provide the stack when aborting suspense boundaries to help you determine which ones were affected by an abort.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
… mode or during prerenders in prod mode (#27761) Historically React would produce component stacks for dev builds only. There is a cost to tracking component stacks and given the prod builds try to optimize runtime performance these stacks were left out. More recently React added production component stacks to Fiber in because it can be immensely helpful in tracking down hard to debug production issues. Fizz was not updated to have a similar behavior. With the advent of prerendering however stacks for production in Fizz are more relevant because prerendering is not really a dev-time task. If you want the ability to reason about errors or postpones that happen during a prerender having component stacks to interrogate is helpful and these component stacks need to be available in production otherwise you are really never going to see them. (it is possible that you could do dev-mode prerenders but we don't expect this to be a common dev mode workflow) To better support the prerender use case and to make error logging in Fizz more useful the following changes have been made 1. `onPostpone` now accepts a second `postponeInfo` argument which will contain a componentStack. Postpones always originate from a component render so the stack should be consistently available. The type however will indicate the stack is optional so we can remove them in the future if we decide the overhead is the wrong tradeoff in certain cases 2. `onError` now accepts a second `errorInfo` argument which may contain a componentStack. If an error originated from a component a stack will be included in the following cases. This change entails tracking the component hierarchy in prod builds now. While this isn't cost free it is implemented in a relatively lean manner. Deferring the most expensive work (reifying the stack) until we are actually in an error pathway. In the course of implementing this change a number of simplifications were made to the code which should make the stack tracking more resilient. We no longer use a module global to curry the stack up to some handler. This was delicate because you needed to always reset it properly. We now curry the stack on the task itself. Another change made was to track the component stack on SuspenseBoundary instances so that we can provide the stack when aborting suspense boundaries to help you determine which ones were affected by an abort. DiffTrain build for commit 63310df.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Historically React would produce component stacks for dev builds only. There is a cost to tracking component stacks and given the prod builds try to optimize runtime performance these stacks were left out. More recently React added production component stacks to Fiber in because it can be immensely helpful in tracking down hard to debug production issues. Fizz was not updated to have a similar behavior.
With the advent of prerendering however stacks for production in Fizz are more relevant because prerendering is not really a dev-time task. If you want the ability to reason about errors or postpones that happen during a prerender having component stacks to interrogate is helpful and these component stacks need to be available in production otherwise you are really never going to see them. (it is possible that you could do dev-mode prerenders but we don't expect this to be a common dev mode workflow)
To better support the prerender use case and to make error logging in Fizz more useful the following changes have been made
onPostponenow accepts a secondpostponeInfoargument which will contain a componentStack. Postpones always originate from a component render so the stack should be consistently available. The type however will indicate the stack is optional so we can remove them in the future if we decide the overhead is the wrong tradeoff in certain casesonErrornow accepts a seconderrorInfoargument which may contain a componentStack. If an error originated from a component a stack will be included in the following cases.This change entails tracking the component hierarchy in prod builds now. While this isn't cost free it is implemented in a relatively lean manner. Deferring the most expensive work (reifying the stack) until we are actually in an error pathway.
In the course of implementing this change a number of simplifications were made to the code which should make the stack tracking more resilient. We no longer use a module global to curry the stack up to some handler. This was delicate because you needed to always reset it properly. We now curry the stack on the task itself.
Another change made was to track the component stack on SuspenseBoundary instances so that we can provide the stack when aborting suspense boundaries to help you determine which ones were affected by an abort.