[Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises #25260
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
sebmarkbage
force-pushed
the
flightthenable
branch
from
f6c0d7a to
0ae7893
Compare
gaearon
reviewed
Sep 14, 2022
sebmarkbage
commented
Sep 14, 2022
| type PendingChunk<T> = { | ||
| status: 'pending', | ||
| value: null | Array<(T) => mixed>, | ||
| reason: null | Array<(mixed) => mixed>, |
There was a problem hiding this comment.
Clever or evil?
There was a problem hiding this comment.
That I'm reusing the value field and reason field to mean resolveListeners and rejectListeners respectively.
acdlite
reviewed
Sep 14, 2022
sebmarkbage
force-pushed
the
flightthenable
branch
from
0ae7893 to
1589d05
Compare
sebmarkbage
commented
Sep 14, 2022
| export function preloadModule<T>(moduleData: ModuleReference<T>): void { | ||
| export function preloadModule<T>( | ||
| moduleData: ModuleReference<T>, | ||
| ): null | Thenable<any> { |
There was a problem hiding this comment.
This changes the protocol of preloading modules. It has to suspend here and not in require module. This is not updated in ReactFlightDOMRelayClientIntegration because they live in www. So someone else has to update it.
I don't think it's currently used there so it's not blocking.
The protocol is that requireModule can no longer suspend by throwing a Promise. Instead, preloadModule should return a Promise that resolves when it's safe to call requireModule synchronously.
@frandiox this will also affect the Vite parts.
sebmarkbage
force-pushed
the
flightthenable
branch
from
1589d05 to
27b9016
Compare
acdlite
approved these changes
Sep 14, 2022
Use the field names used by the Thenable data structure passed to use(). These are considered public in this model. This adds another field since we use a separate field name for "reason".
This doesn't just ping but resolves/rejects with the value.
If a module is blocked, then we can't complete initializing a model. However, we can still let it parse, and then fill in the missing pieces later. We need to block it from resolving until all dependencies have filled in which we can do with a ref count.
We currently loop over all chunks at the end to error them if they're still pending. We shouldn't do this if they're pending because they're blocked on an external resource like a module because the module might not resolve before the Flight connection closes and that's not an error. In an alternative solution I had a set that tracked pending chunks and removed one at a time. While the loop at the end is faster it's more work as we go. I figured the extra status might also help debugging. For modules we can probably assume no forward references, and the first async module we can just use the promise as the chunk. So we could probably get away with this only on models that are blocked by modules.
sebmarkbage
force-pushed
the
flightthenable
branch
from
27b9016 to
1c481e9
Compare
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Sep 22, 2022
Summary: This sync includes the following changes: - **[0cac4d54c](facebook/react@0cac4d54c )**: Double invoked effects on suspended children ([#25307](facebook/react#25307)) //<Samuel Susla>// - **[3d615fc14](facebook/react@3d615fc14 )**: Grammar. Removed doubles of the word "the". ([#25295](facebook/react#25295)) //<Victoria Graf>// - **[6e3bc8a](facebook/react@6e3bc8a2e )**: [DevTools] Check if Proxy exists before creating DispatcherProxy ([#25278](facebook/react#25278)) //<Tianyu Yao>// - **[e7fc04b29](facebook/react@e7fc04b29 )**: [react-dom] Reorganize react-dom internals to match react ([#25277](facebook/react#25277)) //<Josh Story>// - **[0b54e0047](facebook/react@0b54e0047 )**: Handle rejections to avoid uncaught rejections ([#25272](facebook/react#25272)) //<Sebastian Markbåge>// - **[c5d06fdc5](facebook/react@c5d06fdc5 )**: [Flight] Fix Webpack Chunk Loading ([#25271](facebook/react#25271)) //<Sebastian Markbåge>// - **[975b64464](facebook/react@975b64464 )**: [Flight] response.readRoot() -> use(response) ([#25267](facebook/react#25267)) //<Sebastian Markbåge>// - **[60fbb7b14](facebook/react@60fbb7b14 )**: [Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises ([#25260](facebook/react#25260)) //<Sebastian Markbåge>// - **[c91a1e03b](facebook/react@c91a1e03b )**: experimental_useEvent ([#25229](facebook/react#25229)) //<Lauren Tan>// - **[346c7d4c4](facebook/react@346c7d4c4 )**: straightford explicit types ([#25253](facebook/react#25253)) //<Jan Kassens>// - **[3401e9200](facebook/react@3401e9200 )**: useMemoCache implementation ([#25143](facebook/react#25143)) //<Joseph Savona>// - **[0556bab32](facebook/react@0556bab32 )**: [Transition Tracing] More Accurate End Time ([#25105](facebook/react#25105)) //<Luna Ruan>// - **[5fdcd23aa](facebook/react@5fdcd23aa )**: Flow: upgrade to 0.140 ([#25252](facebook/react#25252)) //<Jan Kassens>// - **[5c43c6f02](facebook/react@5c43c6f02 )**: Unwind the current workInProgress if it's suspended ([#25247](facebook/react#25247)) //<Sebastian Markbåge>// - **[e52fa4c57](facebook/react@e52fa4c57 )**: Add early exit to strict mode ([#25235](facebook/react#25235)) //<Samuel Susla>// - **[6aa38e74c](facebook/react@6aa38e74c )**: Flow: enable unsafe-addition error ([#25242](facebook/react#25242)) //<Jan Kassens>// - **[ba7b6f418](facebook/react@ba7b6f418 )**: Flow: upgrade to 0.132 ([#25244](facebook/react#25244)) //<Jan Kassens>// - **[9328988c0](facebook/react@9328988c0 )**: Flow: fix Fiber typed as any ([#25241](facebook/react#25241)) //<Jan Kassens>// - **[c739cef2f](facebook/react@c739cef2f )**: Flow: ReactFiberHotReloading recursive type ([#25225](facebook/react#25225)) //<Jan Kassens>// - **[c156ecd48](facebook/react@c156ecd48 )**: Add some test coverage for some error cases ([#25240](facebook/react#25240)) //<Sebastian Markbåge>// - **[3613284dc](facebook/react@3613284dc )**: experimental_use(context) for server components and ssr ([#25226](facebook/react#25226)) //<mofeiZ>// - **[269c4e975](facebook/react@269c4e975 )**: Prevent infinite re-renders in StrictMode + Offscreen ([#25203](facebook/react#25203)) //<Samuel Susla>// - **[8003ab9cf](facebook/react@8003ab9cf )**: Flow: remove explicit object syntax ([#25223](facebook/react#25223)) //<Jan Kassens>// - **[492c6e29e](facebook/react@492c6e29e )**: Flow: upgrade to 0.127 ([#25221](facebook/react#25221)) //<Jan Kassens>// - **[8a9e7b6ce](facebook/react@8a9e7b6ce )**: Flow: implicit-inexact-object=error ([#25210](facebook/react#25210)) //<Jan Kassens>// - **[37cc6bf12](facebook/react@37cc6bf12 )**: Remove useDeferredValue and useTransition from Flight subset ([#25215](facebook/react#25215)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c28f313...0cac4d5 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D39696377 fbshipit-source-id: 113878d22d6244b8555b5fb86db1da5d43f7cfd9
rickhanlonii
pushed a commit
that referenced
this pull request
Oct 5, 2022
… of throwing Promises (#25260) * [Flight] Align Chunks with Thenable used with experimental_use Use the field names used by the Thenable data structure passed to use(). These are considered public in this model. This adds another field since we use a separate field name for "reason". * Implement Thenable Protocol on Chunks This doesn't just ping but resolves/rejects with the value. * Subclass Promises * Pass key through JSON parsing * Wait for preloadModules before resolving module chunks * Initialize lazy resolved values before reading the result * Block a model from initializing if its direct dependencies are pending If a module is blocked, then we can't complete initializing a model. However, we can still let it parse, and then fill in the missing pieces later. We need to block it from resolving until all dependencies have filled in which we can do with a ref count. * Treat blocked modules or models as a special status We currently loop over all chunks at the end to error them if they're still pending. We shouldn't do this if they're pending because they're blocked on an external resource like a module because the module might not resolve before the Flight connection closes and that's not an error. In an alternative solution I had a set that tracked pending chunks and removed one at a time. While the loop at the end is faster it's more work as we go. I figured the extra status might also help debugging. For modules we can probably assume no forward references, and the first async module we can just use the promise as the chunk. So we could probably get away with this only on models that are blocked by modules.
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: This sync includes the following changes: - **[0cac4d54c](facebook/react@0cac4d54c )**: Double invoked effects on suspended children ([facebook#25307](facebook/react#25307)) //<Samuel Susla>// - **[3d615fc14](facebook/react@3d615fc14 )**: Grammar. Removed doubles of the word "the". ([facebook#25295](facebook/react#25295)) //<Victoria Graf>// - **[6e3bc8a](facebook/react@6e3bc8a2e )**: [DevTools] Check if Proxy exists before creating DispatcherProxy ([facebook#25278](facebook/react#25278)) //<Tianyu Yao>// - **[e7fc04b29](facebook/react@e7fc04b29 )**: [react-dom] Reorganize react-dom internals to match react ([facebook#25277](facebook/react#25277)) //<Josh Story>// - **[0b54e0047](facebook/react@0b54e0047 )**: Handle rejections to avoid uncaught rejections ([facebook#25272](facebook/react#25272)) //<Sebastian Markbåge>// - **[c5d06fdc5](facebook/react@c5d06fdc5 )**: [Flight] Fix Webpack Chunk Loading ([facebook#25271](facebook/react#25271)) //<Sebastian Markbåge>// - **[975b64464](facebook/react@975b64464 )**: [Flight] response.readRoot() -> use(response) ([facebook#25267](facebook/react#25267)) //<Sebastian Markbåge>// - **[60fbb7b14](facebook/react@60fbb7b14 )**: [Flight] Implement FlightClient in terms of Thenable/Promises instead of throwing Promises ([facebook#25260](facebook/react#25260)) //<Sebastian Markbåge>// - **[c91a1e03b](facebook/react@c91a1e03b )**: experimental_useEvent ([facebook#25229](facebook/react#25229)) //<Lauren Tan>// - **[346c7d4c4](facebook/react@346c7d4c4 )**: straightford explicit types ([facebook#25253](facebook/react#25253)) //<Jan Kassens>// - **[3401e9200](facebook/react@3401e9200 )**: useMemoCache implementation ([facebook#25143](facebook/react#25143)) //<Joseph Savona>// - **[0556bab32](facebook/react@0556bab32 )**: [Transition Tracing] More Accurate End Time ([facebook#25105](facebook/react#25105)) //<Luna Ruan>// - **[5fdcd23aa](facebook/react@5fdcd23aa )**: Flow: upgrade to 0.140 ([facebook#25252](facebook/react#25252)) //<Jan Kassens>// - **[5c43c6f02](facebook/react@5c43c6f02 )**: Unwind the current workInProgress if it's suspended ([facebook#25247](facebook/react#25247)) //<Sebastian Markbåge>// - **[e52fa4c57](facebook/react@e52fa4c57 )**: Add early exit to strict mode ([facebook#25235](facebook/react#25235)) //<Samuel Susla>// - **[6aa38e74c](facebook/react@6aa38e74c )**: Flow: enable unsafe-addition error ([facebook#25242](facebook/react#25242)) //<Jan Kassens>// - **[ba7b6f418](facebook/react@ba7b6f418 )**: Flow: upgrade to 0.132 ([facebook#25244](facebook/react#25244)) //<Jan Kassens>// - **[9328988c0](facebook/react@9328988c0 )**: Flow: fix Fiber typed as any ([facebook#25241](facebook/react#25241)) //<Jan Kassens>// - **[c739cef2f](facebook/react@c739cef2f )**: Flow: ReactFiberHotReloading recursive type ([facebook#25225](facebook/react#25225)) //<Jan Kassens>// - **[c156ecd48](facebook/react@c156ecd48 )**: Add some test coverage for some error cases ([facebook#25240](facebook/react#25240)) //<Sebastian Markbåge>// - **[3613284dc](facebook/react@3613284dc )**: experimental_use(context) for server components and ssr ([facebook#25226](facebook/react#25226)) //<mofeiZ>// - **[269c4e975](facebook/react@269c4e975 )**: Prevent infinite re-renders in StrictMode + Offscreen ([facebook#25203](facebook/react#25203)) //<Samuel Susla>// - **[8003ab9cf](facebook/react@8003ab9cf )**: Flow: remove explicit object syntax ([facebook#25223](facebook/react#25223)) //<Jan Kassens>// - **[492c6e29e](facebook/react@492c6e29e )**: Flow: upgrade to 0.127 ([facebook#25221](facebook/react#25221)) //<Jan Kassens>// - **[8a9e7b6ce](facebook/react@8a9e7b6ce )**: Flow: implicit-inexact-object=error ([facebook#25210](facebook/react#25210)) //<Jan Kassens>// - **[37cc6bf12](facebook/react@37cc6bf12 )**: Remove useDeferredValue and useTransition from Flight subset ([facebook#25215](facebook/react#25215)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c28f313...0cac4d5 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D39696377 fbshipit-source-id: 113878d22d6244b8555b5fb86db1da5d43f7cfd9
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.
This moves the Flight Client's to use Chunks that mimick a variant of Promises instead of relying on throwing Promises. It still throws Promises in the bounds (readRoot and the generate React.lazy init) but that can be removed in a follow up.
There are a number of things that I'm not too happy about in this implementation:
The Webpack plugin isn't quite ideal. Not sure if that should even use the status fields.
There is a somewhat of an edge case where a model can reference a module that is still loading. Modules are the only external resource that we're blocked on. This then can turn other models to become blocked too. This was pretty simple but inefficient before. It would just throw and reparse the model every time something pinged. Which was fine because it's an edge case that you use it and even more so that the module isn't already resolved.
The Chunks extend Promise but they're not actually compatible because the the
then()doesn't return another Promise. Similarly, there's no way to lazy initialize without allocating and adding a listener at the same time even though it can synchronously resolve. If we have added listeners before, there's no way to ping and only if it's actually read then initialize it.I'm also not sure if I'm over optimizing over being able to use the built-in Promises more.
It's just way more code.