-
Notifications
You must be signed in to change notification settings - Fork 48.9k
[Flight] Make debug info and console log resolve in predictable order #33665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
type ConsoleEntry = [ | ||
string, | ||
ReactStackTrace, | ||
null | ReactComponentInfo, | ||
string, | ||
mixed, | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type ConsoleEntry = [ | |
string, | |
ReactStackTrace, | |
null | ReactComponentInfo, | |
string, | |
mixed, | |
]; | |
type ConsoleEntry = [ | |
/* eslint-disable no-undef -- eslint does not understand Flow tuple syntax. */ | |
methodName: string, | |
stackTrace: ReactStackTrace, | |
owner: null | ReactComponentInfo, | |
env: string, | |
args: mixed, | |
/* eslint-enable no-undef */ | |
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this error? I'm not seeing any lint errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your version it does not error. I wanted to suggest adding the tuple labels. And then it would error unless we disable eslint. Devon did the same here:
react/packages/react-server-dom-parcel/src/shared/ReactFlightImportMetadata.js
Lines 12 to 20 in 3cfcdfb
export type ImportMetadata = [ | |
// eslint does not understand Flow tuple syntax. | |
/* eslint-disable */ | |
id: string, | |
name: string, | |
bundles: Array<string>, | |
importMap?: {[string]: string}, | |
/* eslint-enable */ | |
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how much trouble we've had with parsers I'm not sure I trust that this won't break linting elsewhere.
0236f23
to
636f359
Compare
stream, | ||
controller, | ||
); | ||
blockedDebugInfo.then(unblock, unblock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is safe because the chunk needs to enter a non-pending state before the next chunk is resolved. Otherwise the next chunk doesn't know it's a stream entry.
Do we block forever or do we give up after a timeout and log a dumbed down value after some time? We could log a placeholder object if it takes to long to resolve the value and set a dedicated property later. We'd end up logging something slightly different but when you have a debugger attached, you could inspect the resolved value later for debuggers where logged values are live (e.g. Chrome DevTools) |
Currently the only thing that can block it is a client reference and it's kind of unusual that you would be able to render anything at all without it. Everything else comes before in the stream anyway. However, the main reason to do this is to allow the separate debug stream. I think for that one, it's up to the framework to ensure that it's live connected and if it closes then we'd fall back to skipping debug info. |
This lets us pass a writable on the server side and readable on the client side to send debug info through a separate channel so that it doesn't interfere with the main payload as much. The main payload refers to chunks defined in the debug info which means it's still blocked on it though. This ensures that the debug data has loaded by the time the value is rendered so that the next step can forward the data. This will be a bit fragile to race conditions until #33665 lands. Another follow up needed is the ability to skip the debug channel on the receiving side. Right now it'll block forever if you don't provide one since we're blocking on the debug data.
React Elements reference debug data (their stack and owner) in the debug channel. If the debug channel isn't wired up this can block the client from resolving. We can infer that if there's no debug channel wired up and the reference wasn't emitted before the element, then it's probably because it's in the debug channel. So we can skip it. This should also apply to debug chunks but they're not yet blocking until #33665 lands.
There's two issues with this approach that I don't like.
Neither of these are an issue for the console part of this since it's an eager side-effect regardless to preserve order and there's nothing else in the stream that depends on a console entry, where as you can depend on a chunk which is blocked by debug info. |
Stacked on #33664.
This resolves an outstanding issue where it was possible for debug info and console logs to become out of order if they up blocked. E.g. by a future reference or a client reference that hasn't loaded yet. Such as if you console.log a client reference followed by one that doesn't. This encodes the order similar to how the stream chunks work.
This also blocks the main chunk from resolving until the last debug info has fully loaded, including future references and client references. This also ensures that we could send some of that data in a different stream, since then it can come out of order.