-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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] Implement useId hook #24172
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, just checking if there's a test for it already!
let currentRequest = null; | ||
|
||
export function prepareToUseHooksForRequest(request: Request) { | ||
currentRequest = request; | ||
} | ||
|
||
export function resetHooksForRequest() { | ||
currentRequest = null; | ||
} | ||
|
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.
@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
: ''; | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebmarkbage I picked S for Server, bikeshed?
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.
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. |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Nits.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking this every useId call, just set it up to an empty string in createRequest.
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.
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.
…ng on the client a server component that used useId
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
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 refetchesin addition to the core change I also modified the structure of options for renderToReadableStream where
onError
,context
, and the newidentifierPrefix
are properties of an Options object argument to avoid the clumsiness of a growing list of optional function arguments.