Store unique ID counter on SSR request object and Add Prefix for separate renders #18576
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 1134a11:
|
lunaruan
force-pushed
the
opaque_id_server_renderer
branch
from
a840c2a
to
ea848da
Compare
Details of bundled changes.Comparing: 53ce0c3...1134a11 react-dom
Size changes (experimental) |
Details of bundled changes.Comparing: 53ce0c3...1134a11 react-dom
Size changes (stable) |
| let serverId: number = 0; | ||
| export function makeServerId(): OpaqueIDType { | ||
| return 'R:' + (serverId++).toString(36); | ||
| export function makeServerId(prefix: ?string, serverId: number): OpaqueIDType { |
There was a problem hiding this comment.
Why is prefix optional? We might as well always pass a string from the outside.
There was a problem hiding this comment.
IIRC we want to minimize the number of bytes we send, so we don't want to append a prefix unless necessary (ie for apps that render multiple subtrees on the server and then stitch them together).
There was a problem hiding this comment.
Flow types suggest "prefix" is optional but "serverId" is required. Is this the intent?
Putting an optional param before a required param seems wrong.
| return new ReactMarkupReadableStream(element, false); | ||
| export function renderToNodeStream( | ||
| element, | ||
| options: PartialRendererOptions | void, |
There was a problem hiding this comment.
Should it be a public API? I suppose people won't know to pass this. Can it be autogenerated or something?
There was a problem hiding this comment.
We won't know when to generate the prefix because we don't want to prepend one unless necessary on the server. Do you have a suggestion on how to autogenerate the prefix
There was a problem hiding this comment.
It's optional. You typically don't need to pass it. You only need it if you're stitching together multiple React SSR in one document.
The Flow type should be options?: PartialRendererOptions. The argument itself is optional but if provided should be an options object. You had that on the other one.
lunaruan
force-pushed
the
opaque_id_server_renderer
branch
3 times, most recently
from
e8f6c8a
to
0477f03
Compare
|
I'm not sure I care that much about the "stitching together" use case. Is it legit? I guess we use it. I'm more concerned about clashes on the client. E.g. two React apps but only one of them uses server rendering. But their IDs clash. Do we have a plan for this? |
| return new ReactMarkupReadableStream(element, false); | ||
| export function renderToNodeStream( | ||
| element, | ||
| options: PartialRendererOptions | void, |
There was a problem hiding this comment.
It's optional. You typically don't need to pass it. You only need it if you're stitching together multiple React SSR in one document.
The Flow type should be options?: PartialRendererOptions. The argument itself is optional but if provided should be an options object. You had that on the other one.
| @@ -78,6 +82,10 @@ import {validateProperties as validateARIAProperties} from '../shared/ReactDOMIn | |||
| import {validateProperties as validateInputProperties} from '../shared/ReactDOMNullInputValuePropHook'; | |||
| import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook'; | |||
|
|
|||
| export type PartialRendererOptions = { | |||
| prefix?: string, | |||
There was a problem hiding this comment.
Which prefix? Maybe call this something to correlate it with the API. E.g. identifierPrefix.
| function useOpaqueIdentifier(): OpaqueIDType { | ||
| return makeServerId(); | ||
| return makeServerId(uniqueIDPrefix, currentUniqueID++); |
There was a problem hiding this comment.
We shouldn't import ReactDOMHostConfig in the server renderer. That's an implementation detail of the client. Just move that function in here in the partial renderer. Then remove makeServerId from all the other host configs.
The partial renderer doesn't have a host config since it's DOM specific but Fizz will have a host config so we can have server renderers for different environments but it'll be a different one than the client uses.
| @@ -838,6 +858,10 @@ class ReactDOMServerRenderer { | |||
|
|
|||
| const prevThreadID = currentThreadID; | |||
| setCurrentThreadID(this.threadID); | |||
| const prevUniqueID = currentUniqueID; | |||
| setCurrentUniqueID(this.uniqueID); | |||
There was a problem hiding this comment.
You pass this value and increment it but you don't actually ever update this.uniqueID. For the synchronous renderToString this won't show up (except maybe for nested calls which we have a open issue about). However for two streaming renders that read multiple times, it'll reset to zero between each read.
You might want to write a unit test that catches this as since it could be pretty bad if we regress on it.
As a solution I'd just pass this instead. setCurrentRenderer(this) and read both the uniqueID, threadID and prefix from the renderer directly.
|
@gaearon The plan for two React apps on the client is to use some kind of global on the client to coordinate this. Unlike the server, we actually have an opportunity to automatically detect that. But let's do it once it comes up? |
To be clear server IDs use a different prefix (uppercase) than the client (lowercase) so that they never collide. The more likely clash is just two React versions on the same document. They'll immediately clash from the first ID. |
|
The client could also use a time or random based prefix. It's not as sensitive to long IDs as the server. |
lunaruan
force-pushed
the
opaque_id_server_renderer
branch
3 times, most recently
from
3eaf00f
to
a269eb7
Compare
| @@ -79,6 +79,10 @@ import {validateProperties as validateARIAProperties} from '../shared/ReactDOMIn | |||
| import {validateProperties as validateInputProperties} from '../shared/ReactDOMNullInputValuePropHook'; | |||
| import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook'; | |||
|
|
|||
| export type PartialRendererOptions = { | |||
There was a problem hiding this comment.
Let's give this type a name that you'd expect the external API to refer to because it's now part of the public API. Flow/TS will likely just copy this name upstream so lets name it what it should have.
ServerOptions?
|
|
||
| export function setCurrentThreadID(threadID: ThreadID) { | ||
| currentThreadID = threadID; | ||
| export let currentPartialRenderer = {}; |
There was a problem hiding this comment.
This type isn't valid. I think it just accidental works since it's an "open type".
Set this to null so it fails if something goes wrong. Let's fool Flow though so you don't need null checks. We want it to fail at runtime "for free".
export let currentPartialRenderer: PartialRenderer = (null: any);
…stinguish between each server render
lunaruan
force-pushed
the
opaque_id_server_renderer
branch
from
a269eb7
to
1134a11
Compare
There is a worry that
useOpaqueIdentifiermight run out of unique IDs if running for long enough. This PR moves the unique ID counter so it's generated per server renderer object instead. For people who render different subtrees, this PR adds aprefixoption torenderToString,renderToStaticMarkup,renderToNodeStream, andrenderToStaticNodeStreamso identifiers can be differentiated for each individual subtree.