[Flight] Integrate Blocks into Flight #18371
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 19435d0:
|
Details of bundled changes.Comparing: fc96a52...19435d0 react-flight-dom-webpack
react-flight-dom-relay
react-client
react-server
Size changes (experimental) |
Details of bundled changes.Comparing: fc96a52...19435d0 react-flight-dom-webpack
react-server
react-flight-dom-relay
react-client
Size changes (stable) |
sebmarkbage
force-pushed
the
blocks
branch
3 times, most recently
from
b5c62c8 to
7072ce6
Compare
| return type(props); | ||
| } else if (typeof type === 'string') { | ||
| // This is a host element. E.g. HTML. | ||
| return [REACT_ELEMENT_TYPE, type, element.key, element.props]; | ||
| } else if (type[0] === REACT_SERVER_BLOCK_TYPE) { |
There was a problem hiding this comment.
What's this? block return value on server only? How will this be wired up? I see you copy pasted it into tests but it's still a bit confusing.
There was a problem hiding this comment.
Ohh I see you're saying the compiler would do that. Okay.
There was a problem hiding this comment.
Yea. I mean I could keep it as $$typeof style objects and then convert it to arrays as part of JSON stringify but I figured this could be a good place to start experimenting with first class tuple style objects instead.
There was a problem hiding this comment.
I think the compiler output should probably depend on a helper that generates this.
I'm thinking react/flight-runtime with a block(...) helper.
| @@ -10,7 +10,11 @@ | |||
|
|
|||
| 'use strict'; | |||
|
|
|||
| const ReactFeatureFlags = require('shared/ReactFeatureFlags'); | |||
There was a problem hiding this comment.
@acdlite Did you say we need to put these inline now? I don't remember why.
There was a problem hiding this comment.
I copy-pasted this from elsewhere so if we do, we should codemod the rest of them too.
| @@ -138,10 +151,23 @@ export function reportGlobalError(response: Response, error: Error): void { | |||
| }); | |||
| } | |||
|
|
|||
| function definePendingProperty( | |||
| function readMaybeChunk<T>(maybeChunk: Chunk<T> | T): T { | |||
| if ((maybeChunk: any).$$typeof !== CHUNK_TYPE) { | |||
There was a problem hiding this comment.
I've noticed maybeChunk is literally the User function in the Relay test. Is this intentional? We're missing some mocking that represents the module transport there?
There was a problem hiding this comment.
It wasn't intentional at first but I did notice it while writing the tests and decided to keep it. I think we should do some mocking here but we don't have it wired up yet in www so we're not sure what we're simulating yet. Once we have it set up we can simulate that here in the mocks.
Also, the tests are far from sufficient yet. Needs more async stuff. I need to write more but don't want to block on it.
| BlockRenderFunction<Props, Data>, | ||
| > = resolveModuleReference(moduleMetaData); | ||
| // TODO: Do this earlier, as the chunk is resolved. | ||
| preloadModule(moduleReference); |
There was a problem hiding this comment.
The comment before implementation of preloadModule says
Returning null means [...]
but retvalue is never used. Is it outdated?
Also, is it completely unobservable? I deleted this and nothing failed.
There was a problem hiding this comment.
The comment is outdated.
It's not unobservable because of this: https://github.com/facebook/react/blob/master/packages/react-flight-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js#L60-L61
Lots more tests needs to be written at some point. None of the tests cover that chunks are being loaded.
| function initializeBlock<Props, Data>( | ||
| tuple: UninitializedBlockPayload<Data>, | ||
| ): BlockComponent<Props, Data> { | ||
| // Require module first and then data. The ordering matters. |
There was a problem hiding this comment.
Can we test correct ordering?
There was a problem hiding this comment.
Hm. Yea. We could test a side-effect in the require somehow while data is still blocked. There's no module function that runs in the tests atm though so needs some infra or hook into the mocks somehow.
There was a problem hiding this comment.
We also don't have any async tests yet for noop which is where we should be adding tests like this.
React elements should no longer be used to extract arbitrary data but only for prerendering trees. Blocks are used to create asynchronous behavior.
It's supposed to pass the original object and not the new one.
This module has shared state. It needs to be external from builds. This lets us test the built versions of the Noop renderer.
packages/react-flight-dom-relay/src/__tests__/ReactFlightDOMRelay-test.internal.js
Show resolved
Hide resolved
|
|
||
| it('can render a server component', () => { | ||
| function Bar({text}) { | ||
| return text.toUpperCase(); |
There was a problem hiding this comment.
Is returning arrays from these supposed to work? I tried wrapping it in [] and got Cannot assign to read only property 'children' of object '#<Object>'
There was a problem hiding this comment.
Hmm actually this only happens when my toEqual fails. Maybe something's wrong with some test helper?
There was a problem hiding this comment.
This is a Jest bug introduced in 25.0.0 jestjs/jest#9531
There was a problem hiding this comment.
Fixed in master but not yet released jestjs/jest#9575
There was a problem hiding this comment.
There's more work to do on server components. If anything it's too permissive and allows anything to serialize here. It should take a separate path that only allows certain renderable things, same as the client.
Somewhat surprising that it doesn't allow an array though.
There was a problem hiding this comment.
Actually I think that particular error actually just a problem with jest logging/matching. When things are not toEqual which includes elements, that are frozen, for some reason there's a write somewhere. So if it doesn't match you get a completely confusing error.
You probably just didn't update the toEqual case to match and then you got this confusing error message instead of "it didn't match".
| @@ -136,7 +163,11 @@ describe('ReactFlightDOM', () => { | |||
| } | |||
|
|
|||
| let {writable, readable} = getTestStream(); | |||
| ReactFlightDOMServer.pipeToNodeWritable(<RootModel />, writable); | |||
| ReactFlightDOMServer.pipeToNodeWritable( | |||
| <RootModel />, | |||
There was a problem hiding this comment.
Should we delete these non-Block tests now? What is their use?
There was a problem hiding this comment.
They're the only ones testing async right now but ideally they should be rewritten to a more idiomatic style.
| while ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| value.$$typeof === REACT_ELEMENT_TYPE | ||
| ) { | ||
| // TODO: Concatenate keys of parents onto children. |
There was a problem hiding this comment.
Server components render away but their keys should still count.
return c ? <Foo key="a" /> : <Foo key="b" />;
function Foo() {
return <div key="meh" />;
}
This renders ['$', 'div', 'meh', null] regardless. But if this is an update request, it should clear the state because the key has changed.
It's not going to be perfectly compatible semantics since the component type should be part of the equation too but that's a bit too much and not necessary since it doesn't have state.
I think a good compromise would be to concatenate the key onto the child. Possibly with some escaping rules. Similar to what React.Children does.
The previous model was centered around JSX rendering random models. This moves to a Block centric approach. To do this, we need a way to pass modules to the client so this wires up the previous infra to resolve modules into meta data.
The server is expected to replace a Block with a compiler to instead return a "Server Block" which contains a ModuleReference and Query. We don't have the compiler parts yet so the tests mimics what the compiler would do for server code.
Now the expected Blocks-first programming model actually works.