[ServerContext] Flight support for ServerContext #23244
Merged
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
salazarm
changed the title
facebook-github-bot
added
the
React Core Team
salazarm
commented
Feb 7, 2022
|
|
||
| const transport = ReactNoopFlightServer.render(<Foo />); | ||
| act(() => { | ||
| ServerContext._currentRenderer = null; |
There was a problem hiding this comment.
I tried using jest.isolateModules so that the instances of ReactServerContext on the server/client were different but that ended up breaking the previous tests for some reason and didn't fix my issue. I can look into a better way to test this but for now is probably sufficient?
There was a problem hiding this comment.
It does surface an interesting scenario. Shopify currently runs Flight and Fizz in the same environment where they could interleave with each other. We don't really support it yet but there's a discussion about whether we should. Perhaps we should flip the isPrimaryRenderer bit for Flight so that if you end up in that situation you could at least make sure one renderer works with both. That might also fix your tests.
|
We won't want to do the convertModelToJSON thing because that creates a deep copy of the whole tree in memory which would be much slower. That doesn't happen with the built-in JSON.stringify since for anything that doesn't have a special override it just walks the existing object and emits it to a string. For Relay we lived with this inefficiency where it converted the whole tree to a copy just because of legacy how this needs to fit into a JSON.stringify call for the GraphQL on the outside anyway but even that's inefficient. This change moves that inefficiency into the non-Relay builds too. I think if we wanted to fix this properly, we'd want to instead reimplement a custom serializer. However, there's another way to solve this which is to always emit a placeholder as if you suspended immediately in the children of all Server Contexts. That has other downsides but only when you actually use Server Contexts instead of for the whole tree. |
|
Gotcha. I had a version working without moving the recursive serialization. Alternatively we could do what JSON.stringify does in userspace by changing the object in place at the tail end of the recursion, no? |
|
I think we shouldn't could this change to the recursion or implement a user space serializer in this first PR. We can do the hack where each provider creates a new task instead since that has to be solved in any scenario anyway. Then we can optimize later. |
sebmarkbage
reviewed
Feb 7, 2022
sebmarkbage
reviewed
Feb 7, 2022
| throw new Error('ServerContext in that name already exists'); | ||
| } | ||
| const context: ReactServerContext<T> = { | ||
| $$typeof: REACT_CONTEXT_TYPE, |
There was a problem hiding this comment.
I haven't fully thought through the implications but I suspect that this will need to have a different symbol so that you can distinguish them.
For one, how are you going to serialize this? There's no way to get to the name but if you add a field here, then it's a different type than the $$typeof represents.
There was a problem hiding this comment.
Yes agreed. I forgot to mention in the summary but this is part 1. In the second part I'll add the hooks and add warnings to make everything work on the client. This was mostly to get all of the tests to pass with this PR. Btw, should this feature be behind a flag?
There was a problem hiding this comment.
Another option is to set the Prototype to something else so that we can easily tell on the client. I think that would be sufficient for us?
There was a problem hiding this comment.
Yea it should be behind a flag. At least in the "isomorphic" and client parts but since that also affects the server (since it depends on isomorphic), you might as well flag it in Flight as well.
We don't use prototypes anywhere else. One reason is that this was faster in benchmarks but another reason is that it works if you have multiple copies of React. That's a little different in this case since the shared state doesn't work properly across multiple copies anyway.
|
Switched back to JSON.stringify + came up with new approach for popping context using a stack |
salazarm
force-pushed
the
flightServerContext
branch
from
0ad0610
to
57b7597
Compare
salazarm
force-pushed
the
flightServerContext
branch
from
db708bb
to
e148a0a
Compare
gaearon
reviewed
Feb 14, 2022
gaearon
reviewed
Feb 15, 2022
salazarm
force-pushed
the
flightServerContext
branch
from
b435021
to
066b206
Compare
salazarm
force-pushed
the
flightServerContext
branch
from
7093fbe
to
7706ed7
Compare
|
Comparing: 6edd55a...b6bbe30 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
|
salazarm
commented
Mar 7, 2022
| @@ -244,7 +245,14 @@ export function popProvider<T>(context: ReactContext<T>): ContextSnapshot { | |||
| } | |||
| } | |||
| if (isPrimaryRenderer) { | |||
| prevSnapshot.context._currentValue = prevSnapshot.parentValue; | |||
| const value = prevSnapshot.parentValue; | |||
| if (value === REACT_SERVER_CONTEXT_DEFAULT_VALUE_NOT_LOADED) { | |||
There was a problem hiding this comment.
Even though this isn't necessary right now since we can't pass Context through context (yet), once we add that, during a refetch we'd be able to send a context back up to the server and useContext it before its initialized. We should add a warning though if it was never initialized at all though, same thing could happen on the client once we add the ability to send the context itself.
sebmarkbage
reviewed
Mar 7, 2022
packages/react/index.stable.js
Outdated
| @@ -22,6 +22,7 @@ export { | |||
| createElement, | |||
| createFactory, | |||
| createRef, | |||
| createServerContext, | |||
There was a problem hiding this comment.
We shouldn't export this in stable yet unless we want it in the RC.
sebmarkbage
reviewed
Mar 7, 2022
| if (value === REACT_SERVER_CONTEXT_DEFAULT_VALUE_NOT_LOADED) { | ||
| prevSnapshot.context._currentValue = | ||
| // $FlowExpectedError - Effectively refined context to ServerContext | ||
| (prevSnapshot.context: ReactServerContext<any>)._defaultValue; |
There was a problem hiding this comment.
You don't have to refine this since _defaultValue is on client contexts too. It's not necessary but doesn't hurt to have it on client contexts too. In theory it could be useful.
sebmarkbage
reviewed
Mar 7, 2022
| if (value === REACT_SERVER_CONTEXT_DEFAULT_VALUE_NOT_LOADED) { | ||
| prevSnapshot.context._currentValue2 = | ||
| // $FlowExpectedError - Effectively refined context to ServerContext | ||
| (prevSnapshot.context: ReactServerContext<any>)._defaultValue; |
sebmarkbage
reviewed
Mar 7, 2022
| @@ -132,9 +137,17 @@ export function popProvider( | |||
| const currentValue = valueCursor.current; | |||
| pop(valueCursor, providerFiber); | |||
| if (isPrimaryRenderer) { | |||
| context._currentValue = currentValue; | |||
| if (currentValue === REACT_SERVER_CONTEXT_DEFAULT_VALUE_NOT_LOADED) { | |||
| context._currentValue = ((context: any): ReactServerContext<any>)._defaultValue; | |||
There was a problem hiding this comment.
This doesn't need refining neither. It could in theory be useful for the client context too in the future. So it's not wrong to apply it to both. Also helps us stay sound(er).
| } else { | ||
| context._currentValue2 = currentValue; | ||
| if (currentValue === REACT_SERVER_CONTEXT_DEFAULT_VALUE_NOT_LOADED) { | ||
| context._currentValue2 = ((context: any): ReactServerContext<any>)._defaultValue; |
sebmarkbage
approved these changes
Mar 8, 2022
sebmarkbage
reviewed
Mar 8, 2022
| return (undefined: any); | ||
| } | ||
|
|
||
| if (value.$$typeof === REACT_PROVIDER_TYPE) { |
There was a problem hiding this comment.
You check for value being truthy in the other if above this but not here. Either it's never null/undefined or you need the check here too. Can it be undefined?
I'd just solve this by moving them into the typeof value === 'object' branch below though so that you only check this on actual objects and only do that check once. They're never null because there's a check above.
There was a problem hiding this comment.
You can also make it a switch statement.
Since you probably match fewer contexts than module references in a tree (given that each child component is a module reference) you can probably move this after the isModuleReference check.
isModuleReference actually also compiles to a $$typeof check so ideally the compiler or VM can optimize to a single switch/jmptable.
…statement in FlightServer
sebmarkbage
reviewed
Mar 8, 2022
| @@ -85,6 +86,8 @@ export default function getComponentNameFromType(type: mixed): string | null { | |||
| } | |||
| if (typeof type === 'object') { | |||
| switch (type.$$typeof) { | |||
| case REACT_SERVER_CONTEXT_TYPE: | |||
| return ((type: any): ReactContext<any>)._globalName + '.Provider'; | |||
There was a problem hiding this comment.
If there is a .displayName we should probably use that like for other contexts and then fallback to _globalName if it doesn't exist.
This should also be wrapped in the feature flag condition and moved to the bottom of the switch. That way it will fallthrough if the condition fails and therefore the case can be removed and therefore the symbol.
sebmarkbage
reviewed
Mar 8, 2022
| return serializeByValueID(providerId); | ||
| } | ||
| case REACT_SERVER_CONTEXT_TYPE: { | ||
| if (key === '__pop') { |
There was a problem hiding this comment.
I'm not sure when in the refactors this happened but this is not quite safe now.
Because if you passed something like <ClientComponent __pop={context} /> it would pop.
It might be better to let the value be a magical value like a specific object reference that the user can't provide.
You don't need the context in the value anyway since it's just a DEV only thing for ourselves to help debugging you can remove that part from popProvider.
There was a problem hiding this comment.
That way you don't even have to check the key.
You can just do if (value === POP).
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Mar 24, 2022
Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 2022
* Flight side of server context * 1 more test * rm unused function * flow+prettier * flow again =) * duplicate ReactServerContext across packages * store default value when lazily initializing server context * . * better comment * derp... missing import * rm optional chaining * missed feature flag * React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED ?? * add warning if non ServerContext passed into useServerContext * pass context in as array of arrays * make importServerContext nott pollute the global context state * merge main * remove useServerContext * dont rely on object getters in ReactServerContext and disallow JSX * add symbols to devtools + rename globalServerContextRegistry to just ContextRegistry * gate test case as experimental * feedback * remove unions * Lint * fix oopsies (tests/lint/mismatching arguments/signatures * lint again * replace-fork * remove extraneous change * rebase * 1 more test * rm unused function * flow+prettier * flow again =) * duplicate ReactServerContext across packages * store default value when lazily initializing server context * . * better comment * derp... missing import * rm optional chaining * missed feature flag * React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED ?? * add warning if non ServerContext passed into useServerContext * pass context in as array of arrays * make importServerContext nott pollute the global context state * merge main * remove useServerContext * dont rely on object getters in ReactServerContext and disallow JSX * add symbols to devtools + rename globalServerContextRegistry to just ContextRegistry * gate test case as experimental * feedback * remove unions * Lint * fix oopsies (tests/lint/mismatching arguments/signatures * lint again * replace-fork * remove extraneous change * rebase * reinline * rebase * add back changes lost due to rebase being hard * emit chunk for provider * remove case for React provider type * update type for SomeChunk * enable flag with experimental * add missing types * fix flow type * missing type * t: any * revert extraneous type change * better type * better type * feedback * change import to type import * test? * test? * remove react-dom * remove react-native-renderer from react-server-native-relay/package.json * gate change in FiberNewContext, getComponentNameFromType, use switch statement in FlightServer * getComponentNameFromTpe: server context type gated and use displayName if available * fallthrough * lint.... * POP * lint
Saadnajmi
pushed a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Jan 15, 2023
Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
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
This PR implements the Server Side support for ServerContext.
Checkout the jest tests to see how its used.
I need to figure out what warnings we need
How did you test this change?
Jest