[Flight] Track Debug Info from Synchronously Unwrapped Promises #33485
Conversation
github-actions
bot
added
the
React Core Team
| // This is a temporary fork of the `use` implementation until we accept | ||
| // promises everywhere. | ||
| const thenable: Thenable<mixed> = (wakeable: any); | ||
| switch (thenable.status) { | ||
| case 'fulfilled': | ||
| case 'fulfilled': { | ||
| forwardDebugInfoFromThenable(request, task, thenable); |
There was a problem hiding this comment.
This only applies to the internal temporary wrapper of Flight.
With this approach the Promise inside React.lazy is never tracked since it disappears and it behaves more like the throwing-a-promise approach. The idea is to convert these to just return Promises eventually though.
You can still assign manual _debugInfo to a lazy though.
However, we also only track Lazy in the child position. Lazy element.type (the only documented approach) are not tracked. So if you have some I/O to load some module with a Component in it, that wouldn't be tracked atm.
| forwardDebugInfo(request, newTask, debugInfo); | ||
| } | ||
| } | ||
| forwardDebugInfoFromCurrentContext(request, newTask, thenable); |
There was a problem hiding this comment.
We should just call forwardDebugInfoFromPromise here. The difference is that if this is not actually a native Promise but is resolved by a thenable which is in turn resolved by I/O (like the test in AsyncDebugInfo-test) or a native Promise then we can pick it up from the context it is resolved in.
The problem is just that now it's inconsistent because such as Thenable will only forward the information if it wasn't already resolved. If it was sync, the information would get lost.
So this is a bit inconsistent about when we expect Thenables to be manually instrumented with debugInfo like the ones coming out of Flight and when we can pick it up automatically.
The ones from FlightClient are also inconsistent in that they may pick up the I/O of the stream that loaded them when they weren't sync but if they were sync then only the forwarded debugInfo from the other server is included.
sebmarkbage
force-pushed
the
iosync
branch
4 times, most recently
from
06d85b7 to
ad831d1
Compare
Unfortunately there's no public API to get to the internal symbols but AsyncResource is conceptually the same class so we can repurpose its method that does the same thing.
So might look for it on thenables when passed to use().
Before we wrap it in a Lazy since it won't be picked up by the lazy.
If we're forwarding debugInfo and also visiting the underlying native Promise, make sure we're not emitting the same debugInfo twice.
This will be used to look at the diff to the next commit
The owner/stack disappears in this case because the use() is not a real await.
Then pass this as the default if there is no deeper await. This means that use() behaves similar to await.
Stacked on #33482.
There's a flaw with getting information from the execution context of the ping. For the soft-deprecated "throw a promise" technique, this is a bit unreliable because you could in theory throw the same one multiple times. Similarly, a more fundamental flaw with that API is that it doesn't allow for tracking the information of Promises that are already synchronously able to resolve.
This stops tracking the async debug info in the case of throwing a Promise and only when you render a Promise. That means some loss of data but we should just warn for throwing a Promise anyway.
Instead, this also adds support for tracking
use()d thenables and forwarding_debugInfofrom then. This is done by extracting the info from the Promise after the fact instead of in the resolve so that it only happens once at the end after the pings are done.This also supports passing the same Promise in multiple places and tracking the debug info at each location, even if it was already instrumented with a synchronous value by the time of the second use.