[Fiber] Transfer _debugInfo from Arrays, Lazy, Thenables and Elements to the inner Fibers.
#28286
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
|
Comparing: 629541b...83d3c62 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
This doesn't track information if the Lazy resolves to |
This was referenced Feb 9, 2024
sebmarkbage
force-pushed
the
debuginfofiber
branch
3 times, most recently
from
6922f83
to
359088d
Compare
gnoff
approved these changes
Feb 12, 2024
| +stack?: string, | ||
| }; | ||
|
|
||
| export type ReactDebugInfo = Array<ReactComponentInfo | ReactAsyncInfo>; |
There was a problem hiding this comment.
The plurality of Info switching when talking about the generic vs inner types is a little confusing. maybe ReactDebugEntries or ReactDebugList?
There was a problem hiding this comment.
I suspect there's a possibility that the inner type moves inwards into an inner set with an outer object that represents an instance of it. So this prepares to not have to rename the outer one.
Comment on lines
+70
to
+73
| // If we have two debugInfo, we need to create a new one. This makes the array no longer | ||
| // live so we'll miss any future updates if we received more so ideally we should always | ||
| // do this after both have fully resolved/unsuspended. | ||
| return outer.concat(inner); |
There was a problem hiding this comment.
should we just let the debug info optionally be a tuple of debug infos that get flattened wherever they get turned into stacks? then we can write into each array indefinitely?
There was a problem hiding this comment.
Yea I considered that but it's also annoying given that this will almost never happen. Since the Flight renderer will have a single row for (almost?) all these cases.
There was a problem hiding this comment.
It is perhaps in conflict with my other point that DebugInfo might become per-instance objects of some sorts too.
Merge any debug info we got from intermediate objects until we create or update a fiber that can hold it.
sebmarkbage
force-pushed
the
debuginfofiber
branch
from
359088d
to
83d3c62
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Feb 12, 2024
…ts to the inner Fibers. (#28286) That way we can use it for debug information like component stacks and DevTools. I used an extra stack argument in Child Fiber to track this as it's flowing down since it's not just elements where we have this info readily available but parent arrays and lazy can merge this into the Fiber too. It's not great that this is a dev-only argument and I could track it globally but seems more likely to make mistakes. It is possible for the same debug info to appear for multiple child fibers like when it's attached to a fragment or a lazy that resolves to a fragment at the root. The object identity could be used in these scenarios to infer if that's really one server component that's a parent of all children or if each child has a server component with the same name. This is effectively a public API because you can use it to stash information on Promises from a third-party service - not just Server Components. I started outline the types for this for some things I was planning to add but it's not final. I was also planning on storing it from `use(thenable)` for when you suspend on a Promise. However, I realized that there's no Hook instance for those to stash it on. So it might need a separate data structure to stash the previous pass over of `use()` that resets each render. No tests yet since I didn't want to test internals but it'll be covered once we have debugging features like component stacks. DiffTrain build for [3f93ca1](3f93ca1)
sebmarkbage
added a commit
that referenced
this pull request
Feb 12, 2024
Alternative to #28295. Instead of stashing all of the Usables eagerly, we can extract them by replaying the render when we need them like we do with any other hook. We already had an implementation of `use()` but it wasn't quite complete. These can also include further DebugInfo on them such as what Server Component rendered the Promise or async debug info. This is nice just to see which use() calls were made in the side-panel but it can also be used to gather everything that might have suspended. Together with #28286 we cover the case when a Promise was used a child and if it was unwrapped with use(). Notably we don't cover a Promise that was thrown (although we do support that in a Server Component which maybe we shouldn't). Throwing a Promise isn't officially supported though and that use case should move to the use() Hook. The pattern of conditionally suspending based on cache also isn't really supported with the use() pattern. You should always call use() if you previously called use() with the same input. This also ensures that we can track what might have suspended rather than what actually did. One limitation of this strategy is that it's hard to find all the places something might suspend in a tree without rerendering all the fibers again. So we might need to still add something to the tree to indicate which Fibers may have further debug info / thenables.
This was referenced Feb 20, 2024
huozhi
added a commit
to vercel/next.js
that referenced
this pull request
Feb 23, 2024
### React upstream changes - facebook/react#28333 - facebook/react#28334 - facebook/react#28378 - facebook/react#28377 - facebook/react#28376 - facebook/react#28338 - facebook/react#28331 - facebook/react#28336 - facebook/react#28320 - facebook/react#28317 - facebook/react#28375 - facebook/react#28367 - facebook/react#28380 - facebook/react#28368 - facebook/react#28343 - facebook/react#28355 - facebook/react#28374 - facebook/react#28362 - facebook/react#28344 - facebook/react#28339 - facebook/react#28353 - facebook/react#28346 - facebook/react#25790 - facebook/react#28352 - facebook/react#28326 - facebook/react#27688 - facebook/react#28329 - facebook/react#28332 - facebook/react#28340 - facebook/react#28327 - facebook/react#28325 - facebook/react#28324 - facebook/react#28309 - facebook/react#28310 - facebook/react#28307 - facebook/react#28306 - facebook/react#28315 - facebook/react#28318 - facebook/react#28226 - facebook/react#28308 - facebook/react#27563 - facebook/react#28297 - facebook/react#28286 - facebook/react#28284 - facebook/react#28275 - facebook/react#28145 - facebook/react#28301 - facebook/react#28224 - facebook/react#28152 - facebook/react#28296 - facebook/react#28294 - facebook/react#28279 - facebook/react#28273 - facebook/react#28269 - facebook/react#28376 - facebook/react#28338 - facebook/react#28331 - facebook/react#28336 - facebook/react#28320 - facebook/react#28317 - facebook/react#28375 - facebook/react#28367 - facebook/react#28380 - facebook/react#28368 - facebook/react#28343 - facebook/react#28355 - facebook/react#28374 - facebook/react#28362 - facebook/react#28344 - facebook/react#28339 - facebook/react#28353 - facebook/react#28346 - facebook/react#25790 - facebook/react#28352 - facebook/react#28326 - facebook/react#27688 - facebook/react#28329 - facebook/react#28332 - facebook/react#28340 - facebook/react#28327 - facebook/react#28325 - facebook/react#28324 - facebook/react#28309 - facebook/react#28310 - facebook/react#28307 - facebook/react#28306 - facebook/react#28315 - facebook/react#28318 - facebook/react#28226 - facebook/react#28308 - facebook/react#27563 - facebook/react#28297 - facebook/react#28286 - facebook/react#28284 - facebook/react#28275 - facebook/react#28145 - facebook/react#28301 - facebook/react#28224 - facebook/react#28152 - facebook/react#28296 - facebook/react#28294 - facebook/react#28279 - facebook/react#28273 - facebook/react#28269 Closes NEXT-2542 Disable ppr test for strict mode for now, @acdlite will check it and we'll sync again
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…ts to the inner Fibers. (facebook#28286) That way we can use it for debug information like component stacks and DevTools. I used an extra stack argument in Child Fiber to track this as it's flowing down since it's not just elements where we have this info readily available but parent arrays and lazy can merge this into the Fiber too. It's not great that this is a dev-only argument and I could track it globally but seems more likely to make mistakes. It is possible for the same debug info to appear for multiple child fibers like when it's attached to a fragment or a lazy that resolves to a fragment at the root. The object identity could be used in these scenarios to infer if that's really one server component that's a parent of all children or if each child has a server component with the same name. This is effectively a public API because you can use it to stash information on Promises from a third-party service - not just Server Components. I started outline the types for this for some things I was planning to add but it's not final. I was also planning on storing it from `use(thenable)` for when you suspend on a Promise. However, I realized that there's no Hook instance for those to stash it on. So it might need a separate data structure to stash the previous pass over of `use()` that resets each render. No tests yet since I didn't want to test internals but it'll be covered once we have debugging features like component stacks.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Alternative to facebook#28295. Instead of stashing all of the Usables eagerly, we can extract them by replaying the render when we need them like we do with any other hook. We already had an implementation of `use()` but it wasn't quite complete. These can also include further DebugInfo on them such as what Server Component rendered the Promise or async debug info. This is nice just to see which use() calls were made in the side-panel but it can also be used to gather everything that might have suspended. Together with facebook#28286 we cover the case when a Promise was used a child and if it was unwrapped with use(). Notably we don't cover a Promise that was thrown (although we do support that in a Server Component which maybe we shouldn't). Throwing a Promise isn't officially supported though and that use case should move to the use() Hook. The pattern of conditionally suspending based on cache also isn't really supported with the use() pattern. You should always call use() if you previously called use() with the same input. This also ensures that we can track what might have suspended rather than what actually did. One limitation of this strategy is that it's hard to find all the places something might suspend in a tree without rerendering all the fibers again. So we might need to still add something to the tree to indicate which Fibers may have further debug info / thenables.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
…ts to the inner Fibers. (#28286) That way we can use it for debug information like component stacks and DevTools. I used an extra stack argument in Child Fiber to track this as it's flowing down since it's not just elements where we have this info readily available but parent arrays and lazy can merge this into the Fiber too. It's not great that this is a dev-only argument and I could track it globally but seems more likely to make mistakes. It is possible for the same debug info to appear for multiple child fibers like when it's attached to a fragment or a lazy that resolves to a fragment at the root. The object identity could be used in these scenarios to infer if that's really one server component that's a parent of all children or if each child has a server component with the same name. This is effectively a public API because you can use it to stash information on Promises from a third-party service - not just Server Components. I started outline the types for this for some things I was planning to add but it's not final. I was also planning on storing it from `use(thenable)` for when you suspend on a Promise. However, I realized that there's no Hook instance for those to stash it on. So it might need a separate data structure to stash the previous pass over of `use()` that resets each render. No tests yet since I didn't want to test internals but it'll be covered once we have debugging features like component stacks. DiffTrain build for commit 3f93ca1.
sebmarkbage
added a commit
that referenced
this pull request
Jun 11, 2024
Stacked on #29804. Transferring of debugInfo was added in #28286. It represents the parent stack between the current Fiber and into the next Fiber we're about to create. I.e. Server Components in between. ~I don't love passing DEV-only fields as arguments anyway since I don't trust closure to remove unused arguments that way.~ EDIT: Actually it seems like closure handled that just fine before which is why this is no change in prod. Instead, I track it on the module scope. Notably with DON'T use try/finally because if something throws we want to observe what it was at the time we threw. Like the pattern we use many other places. Now we can use this when we create the Throw Fiber to associate the Server Components that were part of the parent stack before this error threw. There by giving us the correct parent stacks at the location that threw.
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.
That way we can use it for debug information like component stacks and DevTools. I used an extra stack argument in Child Fiber to track this as it's flowing down since it's not just elements where we have this info readily available but parent arrays and lazy can merge this into the Fiber too. It's not great that this is a dev-only argument and I could track it globally but seems more likely to make mistakes.
It is possible for the same debug info to appear for multiple child fibers like when it's attached to a fragment or a lazy that resolves to a fragment at the root. The object identity could be used in these scenarios to infer if that's really one server component that's a parent of all children or if each child has a server component with the same name.
This is effectively a public API because you can use it to stash information on Promises from a third-party service - not just Server Components. I started outline the types for this for some things I was planning to add but it's not final.
I was also planning on storing it from
use(thenable)for when you suspend on a Promise. However, I realized that there's no Hook instance for those to stash it on. So it might need a separate data structure to stash the previous pass over ofuse()that resets each render.No tests yet since I didn't want to test internals but it'll be covered once we have debugging features like component stacks.