[Flight] Implement useId hook #24172
Merged
+149
−20
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
|
Comparing: 26a5b3c...d72d44d 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
|
| @@ -512,6 +512,59 @@ describe('ReactFlight', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| describe('Hooks', () => { | |||
There was a problem hiding this comment.
Do we have a test for how useId would work for sever components that have children that are client components and need the same ID?
There was a problem hiding this comment.
I don't know if I'm misunderstanding some finer point but my expectation is that if you use useId in Flight you will if necessary pass that id as props to client components like any other prop when needed for some purpose
There was a problem hiding this comment.
Yeah, just checking if there's a test for it already!
gnoff
changed the title
gnoff
commented
Mar 28, 2022
Comment on lines
+16
to
+25
| let currentRequest = null; | ||
|
|
||
| export function prepareToUseHooksForRequest(request: Request) { | ||
| currentRequest = request; | ||
| } | ||
|
|
||
| export function resetHooksForRequest() { | ||
| currentRequest = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
@sebmarkbage there is already setCurrentCache behavior that I could consolidate since they are set and reset at the same points in performWork. For setting the cache I noticed that the previous cache is restored but my understanding of the code is the previous cache will always be null (meaning you cannot have a second performWork start before a first performWork finishes)
If the prevCache restoration is important it makes consolidation of multiple things to set awkward but we could move to just reading the cache from the request given it is set for the duration of performWork and holds the cache
gnoff
commented
Mar 28, 2022
| : ''; | ||
| const id = currentRequest.identifierCount++; | ||
| // use 'S' for Flight components to distinguish from 'R' and 'r' in Fizz/Client | ||
| return ':' + prefix + 'S' + id.toString(32) + ':'; |
There was a problem hiding this comment.
@sebmarkbage I picked S for Server, bikeshed?
sebmarkbage
reviewed
May 27, 2022
There was a problem hiding this comment.
One decision we made was that we recognize that this case won't be semantically equivalent:
<ClientComponent>
<ServerComponent />
</ClientComponent>let ServerComponent = () => <div id={useId} />;
let ClientComponent = ({children}) => <div>{children}{children}</div>;
Since the same ID will be used for both children. We're ok with that. We might add a DEV warning in the future.
|
An unfortunate consequence of using an incrementing counter is that it won't be deterministic but Flight is already not deterministic since it doesn't yet inline suspended points if they unsuspend before flushing. |
sebmarkbage
reviewed
May 30, 2022
| @@ -826,6 +833,7 @@ function performWork(request: Request): void { | |||
| const prevCache = getCurrentCache(); | |||
| ReactCurrentDispatcher.current = Dispatcher; | |||
| setCurrentCache(request.cache); | |||
There was a problem hiding this comment.
Instead of setting up two different contextual variables (and more to come) we should just use the request for the Cache too. That can be a follow up but should fast follow.
While we're add it we should also put the Cache behind the enableCache since it's not coupled to Flight.
sebmarkbage
approved these changes
May 30, 2022
sebmarkbage
requested changes
May 30, 2022
| @@ -102,14 +108,12 @@ export type Request = { | |||
| writtenSymbols: Map<Symbol, number>, | |||
| writtenModules: Map<ModuleKey, number>, | |||
| writtenProviders: Map<string, number>, | |||
| identifierPrefix?: string, | |||
There was a problem hiding this comment.
We should never have optional properties in internal data structures since we want consistent hidden classes. Only short lived and rare config objects and such should have those.
This should just be string always and set to empty string when you initialize it if no option is passed.
sebmarkbage
reviewed
May 30, 2022
| if (currentRequest === null) { | ||
| throw new Error('useId can only be used while React is rendering'); | ||
| } | ||
| const prefix = currentRequest.identifierPrefix |
There was a problem hiding this comment.
Instead of checking this every useId call, just set it up to an empty string in createRequest.
There was a problem hiding this comment.
Would it make sense to go even further and stash the identifierPrefix and identifierCount in module scope alonside the request and then reference prefix / increment count until resetHooks is called and stash the results back on the request?
The approach for ids for Flight is different from Fizz/Client where there is a need for determinancy. Flight rendered elements will not be rendered on the client and as such the ids generated in a request only need to be unique. However since FLight does support refetching subtrees it is possible a client will need to patch up a part of the tree rather than replacing the entire thing so it is not safe to use a simple incrementing counter. To solve for this we allow the caller to specify a prefix. On an initial fetch it is likely this will be empty but on refetches or subtrees we expect to have a client `useId` provide the prefix since it will guaranteed be unique for that subtree and thus for the entire tree. It is also possible that we will automatically provide prefixes based on a client/Fizz useId on refetches in addition to the core change I also modified the structure of options for renderToReadableStream where `onError`, `context`, and the new `identifierPrefix` are properties of an Options object argument to avoid the clumsiness of a growing list of optional function arguments.
sebmarkbage
approved these changes
May 31, 2022
…ng on the client a server component that used useId
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jun 6, 2022
Summary: This sync includes the following changes: - **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>// - **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>// - **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>// - **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>// - **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>// - **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>// - **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>// - **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>// - **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>// - **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>// - **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>// - **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>// - **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>// - **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>// - **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>// - **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>// - **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>// - **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>// - **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>// - **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>// Changelog: [General][Changed] - React Native sync for revisions bd4784c...d300ceb jest_e2e[run_all_tests] Reviewed By: cortinico, kacieb Differential Revision: D36874368 fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
4 tasks
This comment was marked as spam.
This comment was marked as spam.
This was referenced Mar 4, 2023
11 tasks
This was referenced Feb 10, 2024
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.
The approach for ids for Flight is different from Fizz/Client where there is a need for determinancy. Flight rendered elements will not be rendered on the client and as such the ids generated in a request only need to be unique. However since FLight does support refetching subtrees it is possible a client will need to patch up a part of the tree rather than replacing the entire thing so it is not safe to use a simple incrementing counter. To solve for this we allow the caller to specify a prefix. On an initial fetch it is likely this will be empty but on refetches or subtrees we expect to have a client
useIdprovide the prefix since it will guaranteed be unique for that subtree and thus for the entire tree. It is also possible that we will automatically provide prefixes based on a client/Fizz useId on refetchesin addition to the core change I also modified the structure of options for renderToReadableStream where
onError,context, and the newidentifierPrefixare properties of an Options object argument to avoid the clumsiness of a growing list of optional function arguments.