-
Notifications
You must be signed in to change notification settings - Fork 49k
[Flight] Assume __turbopack_load_by_url__
returns a cached Promise
#33792
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
[Flight] Assume __turbopack_load_by_url__
returns a cached Promise
#33792
Conversation
const instrumentedChunks: WeakSet<string> = new WeakSet(); | ||
const loadedChunks: WeakSet<string> = new WeakSet(); |
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.
Question is we still need this or if we're ok with Promise.all([Promise.resolve(undefined)])
instead of Promise.all([null])
. Or we change Turbopack to return null
if the chunk is already loaded.
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.
It avoids potentially a lot of unnecessary overhead when we end up blocking a large number of chunks downstream for just a microtask. Especially on the server where all of these are either fully sync for basically every request or every request will have to go through some of this.
However, if we trust that Turbopack returns cached promises, then we can also instrument those promises with our .status
/.value
convention, or even better, have Turbopack yield already instrumented Promises.
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.
Yeah, I misread the code originally. It's not Promise.all([null])
if the chunk is already loaded. For sync chunks, preloadModule
becomes fully sync. I only accounted for that later in https://github.com/facebook/react/compare/e6ed827a0a52671417a7e34a1707bfda1a209c44..6a4296437e7eb12b18e7b5ee10d213a03cc1b6a1
e6ed827
to
6a42964
Compare
__turbopack_load__
returns a cached Promise__turbopack_load_by_url__
returns a cached Promise
6a42964
to
690f10d
Compare
…facebook#33792) DiffTrain build for [d85ec5f](facebook@d85ec5f)
…facebook#33792) DiffTrain build for [d85ec5f](facebook@d85ec5f)
React doesn't have a signal it can hook into to dispose of chunk cache. Bundlers used to re-evaluate the React Client module to get a fresh cache. However, this is problematic if React Client is in the same bundle (but different module layer) as React Server.
Re-evaluating React Server is not safe since it contains side-effects (console patching and async_hooks).
We can drop the chunk cache in
react-server-dom-turbopack
by assuming Turbopack returns a cached Promise from__turbopack_load_by_url__
which is implemented in vercel/next.js#81663.As a follow-up, Turbopack will return already instrumented Promises (setting
status
).