HostRoot no longer pops context provider during complete phase #8568
Conversation
If I recall correctly I needed the host container to still exist during the commit phase for this. However now that I think of it we would get the wrong one in the portal (root instead of portal root) so it's likely a bug. Can you figure out a failing test for this? |
|
We can also get the wrong one because we could reorder commit phase to not be lined up with the last reconciliation so this seem fragile regardless. We should always popHostContainer in complete and if we need it in commit we should readd it. Although it might be hard to do because the commit phase doesn't happen in top down order. It only visits nodes with side-effects. |
|
I'm not sure how we can solve the problem of need the host container in the commit phase since we'd need to store an extra field for this. Maybe we should simply avoid that in the commit phase and only allow it in the complete phase. We probably want to calculate the props diff in complete anyway. It's currently only used for lazily subscribing to events, which we never undo so it's fine to do that in the complete phase. In fact, I think we might want to stop doing that lazily anyway. |
|
Sounds like there is a bigger question about the host container stuff. Is this diff safe to merge in the meanwhile? (It's related, but not quite the same thing.) |
| @@ -77,6 +77,7 @@ function isContextProvider(fiber : Fiber) : boolean { | |||
| exports.isContextProvider = isContextProvider; | |||
|
|
|||
| function popContextProvider() : void { | |||
| invariant(index >= 0, 'Unexpected context pop'); | |||
There was a problem hiding this comment.
I'd prefer > -1 here because we already compare to -1 in a few parts of the file.
Where are those tests? Are we sure we'll commit them later? Don't want them to get lost. |
|
Sure. Happy to make the nit-change. The tests are already committed. With only the invariant addition, several existing tests fail. With the removal of the superfluous |
|
Ah okay this makes sense. 👍 |
Also added an invariant warning to guard against context being popped too many times.
bvaughn
force-pushed
the
top-level-context-push-pop
branch
from
6d8b3d6
to
b4745ca
Compare
|
Did this fix the redbox we were seeing? |
|
Fixed some problems, not all. I think I have a fix for some more (still not all) but need to step through the failing tests to determine if it's really a causal-fix or just a symptom-fix. |
In the begin work phase, we call
pushContextProviderforClassComponents. In the complete work phase, we callpopContextProviderfor bothClassComponents andHostRoots. The result is that we end up popping more times than we should.This PR removes the
popContextProvidercall forHostRootand adds an invariant warning topopContextProvideras well.Note that no new tests fail with the new invariant warning once the superfluous
popContextProvidercall is removed. (Several fail with it in place.)Note also that in the begin work phase we call
pushHostContainerforHostRoots andHostPortals but we only callpopHostContainerforHostPortals during the complete work phase. It seems like we should align these calls as well but I don't have enough context to tackle that yet.