HostRoot no longer pops context provider during complete phase

[bvaughn]

In the begin work phase, we call pushContextProvider for ClassComponents. In the complete work phase, we call popContextProvider for both ClassComponents and HostRoots. The result is that we end up popping more times than we should.

This PR removes the popContextProvider call for HostRoot and adds an invariant warning to popContextProvider as well.

Note that no new tests fail with the new invariant warning once the superfluous popContextProvider call is removed. (Several fail with it in place.)

Note also that in the begin work phase we call pushHostContainer for HostRoots and HostPortals but we only call popHostContainer for HostPortals 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.

[gaearon]

Note also that in the begin work phase we call pushHostContainer for HostRoots and HostPortals but we only call popHostContainer for HostPortals during the complete work phase.

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?

[sebmarkbage]

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.

[sebmarkbage]

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.

[bvaughn]

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.)

[gaearon]

I'd prefer > -1 here because we already compare to -1 in a few parts of the file.

[gaearon]

LGTM but I'd like my tiny nit changed

[gaearon]

Note that no new tests fail with the new invariant warning once the superfluous popContextProvider call is removed. (Several fail with it in place.)

Where are those tests? Are we sure we'll commit them later? Don't want them to get lost.

[bvaughn]

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 popContextProvider, they no longer fail. Sorry if my initial comment was confusing.

[gaearon]

Ah okay this makes sense. 👍

[sophiebits]

Did this fix the redbox we were seeing?

[bvaughn]

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.