[Fax] Support nesting in existing RSC renderers #30736
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
React Core Team
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
4639462
to
7b4f777
Compare
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
7b4f777
to
627e402
Compare
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
627e402
to
af10dfd
Compare
eps1lon
changed the title
react-markup in Flight fixture
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
af10dfd
to
11ed7c3
Compare
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
11ed7c3
to
146c928
Compare
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
146c928
to
c334f20
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
From what I can tell they're not just broken at the dispatcher level but also with regards to |
Just keep in mind that this blocks the RSC DevTools stuff and enableOwnerStacks if they end up getting installed in the wrong order. |
| @@ -452,6 +452,7 @@ export type Dispatcher = { | |||
| }; | |||
|
|
|||
| export type AsyncDispatcher = { | |||
| getActiveCache: () => Map<Function, mixed> | null, | |||
| getCacheForType: <T>(resourceType: () => T) => T, | |||
There was a problem hiding this comment.
There's no need for both of these. If we're saying that the implementation detail is a Map<Function> and that is exposed through getActiveCache(), there's not that much getCacheForType can configure and it's unnecessary indirection. getCacheForType can just be a utility function that calls getActiveCache. (Or we can get rid of getCacheForType completely and just have the callers call getActiveCache.) No need to be on the dispatcher anymore.
There was a problem hiding this comment.
To preempt "I'll do it in a follow up". Since we know that these dispatchers are messed with by people and we break them we don't want to change this too many times. Even the async cache ones (cc Janka).
There was a problem hiding this comment.
Like this f140f36 (#30736)?
Whatever resolves the cache conflict needs to know the previous dispatcher so it made sense to me to just do the resolution inside getActiveCache. I don't know how else to do it if we want getActiveCache to return the actual Map (or null) and resolve conflicts in a utility since that utility needs to know the current and previous dispatcher.
Yeah, I'll get it right in this one. |
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
2318712
to
f140f36
Compare
|
|
||
| // If both caches are active, reads will prefer the outer cache | ||
| // Writes will go into both caches. | ||
| const chainedCache: AsyncCache = { |
There was a problem hiding this comment.
This is a hot path so we shouldn't allocate in it. Instead we should be able to structure it so that we don't need to.
For example if this is the logic we want, then a better approach might be to just have to methods on the dispatcher. One for get and one for set.
However, I wonder if it's not better to just delegate to one cache over another.
It's not also not good that we don't preserve hidden classes here. It should either always be a WeakMap or never a WeakMap. For example if you load a graph first and then do a bunch of renders and then later load react-markup you'd get a bunch of hidden class breakages causing recompilation but also it'd not assume that it might be either WeakMap or this other thing so all lookups would be megamorphic instead of just checking the one.
There was a problem hiding this comment.
What if when we start a new request, we check if we're already inside someone else's render (the dispatcher returns a cache) and if so we just use their cache?
It seems reasonable that an inner render shares the cache of an outer render.
There was a problem hiding this comment.
I had the same concerns but couldn't see a way to make it work when chaining at module evaluation time. Deferring to the existing cache if it exists seems doable (ignoring the ownerstack for now which I need to reconcile still): 2ecf3ec (#30736)
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
7489d30
to
0b38251
Compare
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
0b38251
to
2ecf3ec
Compare
eps1lon
force-pushed
the
fax/nested-rsc-renderer
branch
from
2ecf3ec
to
35507c5
Compare
Summary
This chains the async dispatchers similarly to what we did #28488.
The cache between nested renderers is shared prioritising the one that was first registered during module evaluation (an earlier version cached in both but I couldn't think of a case where we need this behavior).
Parent stacks and Owner stacks currently don't work in
onErrorofreact-markup. I'd need to think about this more but it's dev-only.Test plan