[Fizz] Ensure Resumable State is Serializable #27388
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
force-pushed
the
serializablestate
branch
2 times, most recently
from
7e58de5
to
d14b721
Compare
| @@ -152,6 +152,7 @@ export type RenderState = { | |||
| fontPreloads: Set<PreloadResource>, | |||
| highImagePreloads: Set<PreloadResource>, | |||
| // usedImagePreloads: Set<PreloadResource>, | |||
| // TODO: Precedences needs to be seeded with the order of precedence after resuming. | |||
There was a problem hiding this comment.
@gnoff Is it necessary for this to be in order after the shell flushes? Because afterwards we just insert relative to existing ones, right? So the order doesn't matter anymore.
There was a problem hiding this comment.
This is correct. we'd only need to reconstruct the right order when we support preamble only and want to resume the shell
| const resourceProps = stylesheetPropsFromRawProps(props); | ||
| const preloadResource = resumableState.preloadsMap.get(key); | ||
| const preloadResource = resumableState.preloadsMap.get(key); // TODO |
There was a problem hiding this comment.
The preload map tracks state which we might need.
| @@ -2182,7 +2182,7 @@ function pushLink( | |||
| precedenceSet.add(resource); | |||
| } | |||
| if (renderState.boundaryResources) { | |||
| renderState.boundaryResources.add(resource); | |||
| renderState.boundaryResources.add(resource); // TODO | |||
There was a problem hiding this comment.
@gnoff Do we need to be able to transfer this to the boundary? We might not need to track them across resuming if assume that they're always flushed in the shell. We might need to keep track of them during a single render pass. Like if we the same resource is discovered in a different boundary which then gets completed first.
There was a problem hiding this comment.
We don't need to track across resuming b/c all style resources should flush with the shell. But within the resume render we still need to track this association b/c the first boundary to complete that depends on this resource needs to create it as part of the reveal instruction and that could be a different boundary than the one that first observed it. (I think this is what you are saying but not sure so I'm restating it)
| pushStyleContents(resource.chunks, props); | ||
| } | ||
| if (renderState.boundaryResources) { | ||
| renderState.boundaryResources.add(resource); // TODO | ||
| } |
There was a problem hiding this comment.
@gnoff I moved this out because it appears to be inconsistent with stylesheet. Either we have to add it to boundary resources each time it's discovered or maybe not all.
If we don't need this to be coordinated because it's always sync available and will always flush early, then maybe we never add it to boundaryResources but if we need it to be emitted, then it seems like we must always add it.
There was a problem hiding this comment.
Yeah I think this was just my mistake. however, we should reconcider whether it makes sense to hold back some style tags when completing boundaries vs just flushing everything we've accumulated so far. If we always flush all style precedences on each tick then we don't need to track
sebmarkbage
force-pushed
the
serializablestate
branch
2 times, most recently
from
30539e6
to
60f646e
Compare
sebmarkbage
force-pushed
the
serializablestate
branch
from
60f646e
to
87ad433
Compare
| precedenceSet.add(resource); | ||
| renderState.stylePrecedences.set(precedence, resource); | ||
| const stylesInPrecedence: Map<string, StyleResource> = new Map(); | ||
| stylesInPrecedence.set(key, resource); |
There was a problem hiding this comment.
for consistency the key should probably be '' here to match the empty style resource created upon a new precedence in pushLink and preinitStyle. The resource isn't really keyed by this key uniquely since it will accumulate contents from many keys
Bootstrapscripts, external runtime and resource queues are all expected to flush in the prerender. They're only needed by the prerender and then they can get dropped/reset. Threfore we can move them to the renderState.
We need some way to look up an existing resource to add it to boundary resources, if it still isn't flushed. Since style tags are 1:1 with precedence, we can just use the precedence to get it. Since we can't get stylesheet resources from stylesMap anymore we have to get it from somewhere and we already have it on the precedence list. We'd just need to iterate to find it and even then we don't have it by key. So I just turn the Set into a Map so we can get it from there.
We have to ensure these are serializable. We transfer them to the resume so that they can be applied to resources discovered during the resume.
sebmarkbage
force-pushed
the
serializablestate
branch
from
87ad433
to
f6d83b1
Compare
Moves writing queues to renderState. We shouldn't need the resource tracking's value. We just need to know if that resource has already been emitted. We can use a Set for this. To ensure that set is directly serializable we can just use a dictionary-like object with no value. See individual commits for special cases. DiffTrain build for [b775564](b775564)
### React upstream changes - facebook/react#27417 - facebook/react#27408 - facebook/react#27409 - facebook/react#27405 - facebook/react#27375 - facebook/react#27407 - facebook/react#27365 - facebook/react#27399 - facebook/react#27395 - facebook/react#27394 - facebook/react#27397 - facebook/react#26992 - facebook/react#27388 - facebook/react#27373 - facebook/react#27332
Moves writing queues to renderState. We shouldn't need the resource tracking's value. We just need to know if that resource has already been emitted. We can use a Set for this. To ensure that set is directly serializable we can just use a dictionary-like object with no value. See individual commits for special cases.
Moves writing queues to renderState. We shouldn't need the resource tracking's value. We just need to know if that resource has already been emitted. We can use a Set for this. To ensure that set is directly serializable we can just use a dictionary-like object with no value. See individual commits for special cases. DiffTrain build for commit b775564.
Moves writing queues to renderState.
We shouldn't need the resource tracking's value. We just need to know if that resource has already been emitted. We can use a Set for this. To ensure that set is directly serializable we can just use a dictionary-like object with no value.
See individual commits for special cases.