[RFC] useServerContextsForRefetch #23382
Closed
Conversation
This file contains hidden or 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
facebook-github-bot
added
CLA Signed
React Core Team
salazarm
force-pushed
the
serverContextHooks
branch
from
fe700d1 to
5aa5554
Compare
|
Comparing: be229c5...6e9f716 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
|
sebmarkbage
reviewed
Mar 1, 2022
| <ServerContextContextProvider value={context}> | ||
| <ServerContext.Provider value={value}>{children}</ServerContext.Provider> | ||
| </ServerContextContextProvider> | ||
| return React.createElement( |
There was a problem hiding this comment.
Prefer React.jsx since that's now what JSX compiles to. Really people shouldn't do that themselves since we can change the compiler but we are us.
sebmarkbage
reviewed
Mar 1, 2022
sebmarkbage
reviewed
Mar 1, 2022
|
|
||
| const {useContext, useMemo, createContext} = React; | ||
|
|
||
| const ServerContextContext = createContext(null); |
There was a problem hiding this comment.
Thought provoking, but could this itself be a ServerContext? 🤯
If we wanted to enable this same hook to work from inside Server Components on the server so that you can serialize the context for later use by other server components (like pre-rendering other subtrees).
|
Now that I see it, I'm not sure about the name. It's partly for refetches but it's also for loading alternative branches of a subtree. That's kind of a refetch but also not quite. Let the bikeshedding commence. |
salazarm
force-pushed
the
serverContextHooks
branch
2 times, most recently
from
adfcd87 to
6228562
Compare
salazarm
force-pushed
the
serverContextHooks
branch
from
6228562 to
73065cc
Compare
sebmarkbage
reviewed
Mar 8, 2022
| @@ -0,0 +1,10 @@ | |||
| /** | |||
There was a problem hiding this comment.
You'll need to add this to be built as a bundle too.
We'll also need a react-server-dom-relay one for FB.
There was a problem hiding this comment.
You can also make tests out of these two in ReactFlightDOM-test and ReactFlightDOMRelay-test to ensure that you did. E.g. by porting the test you have above.
sebmarkbage
reviewed
Mar 8, 2022
packages/react-client/package.json
Outdated
| "exports": { | ||
| ".": "./flight.js", | ||
| "./flight": "./flight.js", | ||
| "./flight-hooks": "./flight-hooks.js" |
There was a problem hiding this comment.
This package is really just exposed as a way to build custom renderers but we don't even publish it.
You don't have to include an exports but if you do you need to include ./src/* to export the source to the compiler. https://github.com/facebook/react/blob/main/packages%2Freact%2Fpackage.json#L30
It gets stripped out on publish.
sebmarkbage
reviewed
Mar 8, 2022
| if ((tuple[1]: any).$$typeof === REACT_PROVIDER_TYPE) { | ||
| return createElement(ServerContextWrapper, null, { | ||
| ServerContext: ((tuple[1]: any): ReactProviderType<any>)._context, | ||
| ...tuple[3], |
There was a problem hiding this comment.
You know it's going to be children and value, so you can just enumerate those here. It's faster than spread.
|
Hi @salazarm! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
sebmarkbage
force-pushed
the
serverContextHooks
branch
from
d581807 to
017e9f4
Compare
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
sebmarkbage
force-pushed
the
serverContextHooks
branch
2 times, most recently
from
c3cbcb6 to
24b3257
Compare
sebmarkbage
force-pushed
the
serverContextHooks
branch
from
24b3257 to
fe1d427
Compare
This is so we know which host config we're using for these. After a couple of future fixes I don't think I'll need this anymore but in the current approach it's needed.
This is not published to npm. It's meant for use in FB only.
So we import the deep import from these entry points instead. Build react-client/flight-hooks For use by implementors of renderers.
We don't support that syntax in the build toolchain.
This surfaces a suspected bug since we build flight-hooks separately it creates a separate copy of the client contexts.
sebmarkbage
force-pushed
the
serverContextHooks
branch
from
fe1d427 to
6e9f716
Compare
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.
Summary
Summary
This PR is stacked on top of #23244 (curse you github for not supporting stacked PRs!) If you want to see just this PR check out this commit: 5aa5554
This PR adds a client side hook to be used in conjunction with Flight/Fizz for using ServerContext.
useServerContextsForRefetch
This hook is meant to be used with Flight (RSC).
It returns the parent server contexts of the react component it is called for.
It returns the original server values, that is if the server context is overridden on the client that value will be ignored.
It returns the server contexts as a reverse linked list in the format:
eg:
Server.js
On the client if we wanted to re-render HybridComponent2 then we'd call the callback returned by useServerContextsForRefetch for HybridComponent2 and it would return [['ServerContext', 'Bar']]. Then on the server we can render using that context.
Note: useServerContextsForRefetch hook only makes sense for Hybrid components and not client components because trying to refetch a client only component with Flight would just result in Flight deferring to the client to render the component. However if you pass the result from Flight to Fizz to SSR the result then that could make sense for devices with really slow CPU however it usually isn't the right tradeoff since the components are already loaded on the client.
Why Refetch at all?
The main benefit to refetching a subtree in Flight rather than just re-rendering on the client can be seen when we consider request waterfalls. A request waterfall on the client includes latency from server <--> client multiple times. Where as a request waterfall on the server is a lot faster since the requests are all happening within your network.
How did you test this change?
jest