Clean up inconsistent naming in Flight implementation #26356
Conversation
| @@ -41,7 +41,7 @@ export type JSONValue = | |||
|
|
|||
| const PENDING = 'pending'; | |||
| const BLOCKED = 'blocked'; | |||
| const RESOLVED_MODEL = 'resolved_model'; | |||
| const RESOLVED_VALUE = 'resolved_value'; | |||
There was a problem hiding this comment.
These are not like the others. E.g. A Module is also a ReactClientValue so why isn't resolved_module also this? We need some way to differentiate between different kinds of values in this context.
There was a problem hiding this comment.
This comment I found difficult to tackle, and I had a hard time finding the best name for this type of chunk. TL;DR: I went with RESOLVED_JSON_VALUE, but I'm am open for other suggestions.
The main problem I have with it is that already before model and module weren't really the best names. Because as you said, a module is also a ReactClientValue, so previously a Model. But ReactClientValue is named from the perspective for the encoder (flight server or flight reply client), whereas this code here is from the perspective of the decoder (flight client or flight reply server), where value (before: model) is something else than ReactClientValue. The value of a module chunk is a client reference object. The value of the chunk in question is an UninitializedValue. So in the streaming client it's a JSON string, and in the relay clients it's a JSONValue. Therefore I went with RESOLVED_JSON_VALUE, so a resolved JSON value that can be initialised (i.e. parsed) next. This is also somewhat in line with how it's modeled in the flight server, where there are completedImportChunks, completedJSONChunks (and completedErrorChunks). Maybe the import chunks in the flight server should even be renamed to module chunks?
I'm really glad for the issue you've pointed out here because it made dive deeper into the whole flight implementation, and I now understand a lot more about how RSC works than before.
| @@ -39,7 +39,7 @@ declare module 'ReactFlightDOMRelayServerIntegration' { | |||
| import type {JSResourceReference} from 'JSResourceReference'; | |||
|
|
|||
| declare export opaque type Destination; | |||
| declare export opaque type BundlerConfig; | |||
There was a problem hiding this comment.
We can't change these without changing the internal code in Meta's codebase. So someone at Meta needs to do that both at the same time.
There was a problem hiding this comment.
Not sure if its connected. But Relay has cleaned up its flight implementation: facebook/relay@689dcb1
There was a problem hiding this comment.
Reverted to BundlerConfig and added a TODO.
|
@sebmarkbage I appreciate you taking the time to review these changes! As for your two review comments, do you think I should try to address them in some way, or would you rather I close the PR? I also understand that this is pretty intrusive since you are actively working on the feature, so no hard feelings either way. |
|
This is great. I was thinking the same thing the other day. So if you can just address the two outstanding issues, we can land. |
unstubbable
force-pushed
the
flight-naming-inconsistencies
branch
from
a13bf97
to
36e576f
Compare
|
I've squashed the changes in the Flight Reply implementation (which came after this PR) into the first two commits. I also have them as separate commits locally. Let me know if I should push that instead. |
| createModelResolver(parentChunk, parentObject, key), | ||
| createModelReject(parentChunk), | ||
| createValueResolver(parentChunk, parentObject, key), | ||
| createValueReject(parentChunk), |
There was a problem hiding this comment.
I think it should be either createValueResolver/createValueRejecter or createValueResolve/createValueReject.
Since `ClientManifest` and `SSRManifest` are now clearly differentiated in their structure (via facebook#26300) and type names (via facebook#26351), this opened up the opportunity to clean up some inconsistencies in how parameters, variables and properties were named (e.g. `config`, `bundlerConfig`, `moduleMap`, `webpackMap`, ...). To improve readability and avoid confusion between the two different types of objects we now always name them either `clientManifest` or `ssrManifest`.
After `ReactModel` has been renamed to `ReactClientValue` in facebook#26351, I think we should also rename the params, variables and functions that handle this type accordingly from `model` to `value`.
…light Reply server
…ple` to `parseJSONValueTuple`
unstubbable
force-pushed
the
flight-naming-inconsistencies
branch
from
e13f90d
to
8ff268a
Compare
Summary
Since
ClientManifestandSSRManifestare now clearly differentiated in their structure (via Simplify Webpack References by encoding file path + export name as single id #26300) and type names (via Split out ServerReferenceMetadata into Id and Bound Arguments #26351), this opened up the opportunity to clean up some inconsistencies in how parameters, variables and properties were named (e.g.config,bundlerConfig,moduleMap,webpackMap, ...). To improve readability and avoid confusion between the two different types of objects we now always name them eitherclientManifestorssrManifest.After
ReactModelhas been renamed toReactClientValuein Split out ServerReferenceMetadata into Id and Bound Arguments #26351, I think we should also rename the params, variables and functions that handle this type accordingly frommodeltovalue.How did you test this change?
yarn testandyarn test --prod