[Flight]: Client-side registerServerReference must not break .bind()#32565
Conversation
As a follow-up for facebook#32534 this ensures that calling `registerServerReference` from the client builds with a server reference does not overwrite the `bind` method that was previously defined by the Flight Server. As was already the case with facebook#32534, the compiler must ensure that the client-side `registerServerReference` is called after the server-side `registerServerReference` is called.
github-actions
Bot
added
the
React Core Team
| Function, | ||
| {id: ServerReferenceId, bound: null | Thenable<Array<any>>}, | ||
| > = new WeakMap(); | ||
| type ServerReferenceMetaData = { |
There was a problem hiding this comment.
The terminology here is a bit unclear but ClientReferenceMetadata is actually the equivalent of ServerReferenceId. I.e. excluding the bound arguments. We should really have bound client references too.
Originally I intended to refer to the Metadata + Bound arguments as a "Reference" inside the payload itself because it's overloaded both in terms of the first class object exposed to user space and references to various IDs in the payload format.
This pairing of bound arguments + the id/metadata should probably have its own name. Maybe ServerReferenceClosure?
| // 'react-server/src/ReactFlightServerConfig', but that can only be imported | ||
| // in a react-server environment. | ||
| function isServerReference(reference: Object): boolean { | ||
| return reference.$$typeof === Symbol.for('react.server.reference'); |
There was a problem hiding this comment.
This isn't good enough to support environments that have custom server reference first class types (like Meta does and all bundlers ideally should have).
But I doubt brand checking is actually the right strategy anyway.
| } | ||
| // Expose encoder for use by SSR, as well as a special bind that can be used to | ||
| // keep server capabilities. | ||
| if (usedWithSSR) { |
There was a problem hiding this comment.
$$FORM_ACTION and $$IS_SIGNATURE_EQUAL should never be included in a non-SSR build and we definitely don't want to include the extra code in browser builds sent to clients.
In fact, we really need to build a separate version of the Flight Clients for react-server and non-react-server environments because these should really never be in any react-server environment.
We cheat in other places like:
https://github.com/facebook/react/blob/main/packages/react-client/src/ReactFlightClient.js#L104-L113
This is going to get us in trouble eventually.
| registerBoundServerReference(reference, id, null, encodeFormAction); | ||
| const bound = | ||
| isServerReference(reference) && reference.$$bound | ||
| ? Promise.resolve(reference.$$bound) |
There was a problem hiding this comment.
We can't make these assumptions about the structure of a Server Reference here. It's up to the bundler.
In fact, these should not even be expandos. I've been meaning to move all the fields on Server/Client References onto WeakMaps since they're internal state owned by the server and should be opaque to the surrounding environment - including Flight's Client.
There was a problem hiding this comment.
Also, if it is bound already then it's used incorrectly and should be an error (if it was even possible to check).
Because registerServerReference should always be called on the original function, not the closure. It represents the raw function and not the closure.
I.e. to create a bound function you need to pass the original function to both both registerServerReferences before binding it.
ReactFlightClient.registerServerReference(ReactFlightServer.registerServerReferenace(fn, ...), ...).bind(...)
| function defineBind<T: Function>(reference: T) { | ||
| // TODO: Instead of checking if it's a server reference, we could also use | ||
| // `reference.hasOwnProperty('bind')`. | ||
| const originalBind = isServerReference(reference) |
There was a problem hiding this comment.
We don't even have to check. We can just assume that it has a bind since it's a function. Otherwise something has gone terribly wrong and if not we probably shouldn't add one.
The principle here is this should be opaque to the environment and as long as we just call through it should work as usual.
In fact, you should really be able to have the same function in multiple server environments too which would be possible if we moved the expandos to WeakMaps.
So this could be bound for the Flight Client, Flight Server ESM and Flight Webpack all at once.
| ? reference.bind | ||
| : FunctionBind; | ||
|
|
||
| function bind(): Function { |
There was a problem hiding this comment.
This shouldn't use a closure. You should be able to have referenceA.bind === referenceB.bind and eventually this should just go on the Function.prototype. Conceptually it's on the Function.prototype and the more native it moves into bundlers and then in the browser it will be.
It's also important semantically because you should be getting all information about the reference from this and not anything from the closure. Native functions aren't bound to anything.
[diff facebook/react@0ca3deeb...6aa8254b](facebook/react@0ca3dee...6aa8254) <details> <summary>React upstream changes</summary> - facebook/react#32465 - facebook/react#32540 - facebook/react#32565 </details>
4 participants
This is a follow-up for #32534 to ensure that calling
registerServerReferencefrom the client builds with a server reference does not overwrite thebindmethod that was previously defined by the Flight Server.As was already the case with #32534, the compiler must ensure that the client-side
registerServerReferenceis called after the server-sideregisterServerReferenceis called.