[Fiber] Remove array indirection in host context #8544
Conversation
| function getRootHostContainer() : C { | ||
| if (containerDepth === -1) { | ||
| throw new Error('Expected to find a root container.'); | ||
| if (rootInstance == null) { |
There was a problem hiding this comment.
For the new Context API I was thinking that we should require a default value so that the type is always guaranteed to be satisfied and the user doesn't have to add extra null checks. Maybe we should do the same for host context?
| contextFibers, | ||
| contextValues, | ||
| contextDepth, | ||
| }; |
There was a problem hiding this comment.
Why does context arrays have to be stored separately here? Can't you just push another context onto the stack of contexts? Even if that is null. Then pop it on the way back up.
There was a problem hiding this comment.
I'd need to know how many can I pop. If I make null the sentinel then I need to somehow forbid renderers from specifying something like string | null as type argument. Is this even possible with Flow?
There was a problem hiding this comment.
Oh never mind, I see how this could work.
This lets us keep subtrees separated without maintaining independent context arrays for subtrees.
|
Addressed #8544 (comment) in 6463d67. |
| rootInstance = portalStack[portalDepth]; | ||
| portalStack[portalDepth] = null; | ||
| portalDepth--; | ||
| // If we pushed any context while in a portal, we need to roll it back. |
There was a problem hiding this comment.
How can you ever pop a host container with a context deeper than one null? Wouldn't this always be exactly one null?
Because any new context provider will be below the portal in the tree and pop on the way up to the portal.
Think of this problem in terms of a normal execution stack. You wouldn't need multiple arrays and these special cases for unwinding in a normal execution stack other than when you unwind for errors so you shouldn't need it here.
I was trying to be smart but didn't need to.
| containerDepth--; | ||
| } | ||
|
|
||
| function getHostContext() : CX | null { |
There was a problem hiding this comment.
Ideally getHotsContext could just return a value from the closure instead of going through the array just like getRootHostContainer does.
3 participants
As discussed in #8490 (comment).