Error from orphaned elements trying to render

[spiffytech]

I'm getting this error in a certain code path is my application:

The rerender() function was called on a node without a parent element.

I can't manage to create a minimal reproduction for this, and I've spent a bunch of hours trying to figure out what's going on here, even had a friend help debug the forgo source code tonight, but I haven't figured it out. I'm hoping you have an idea.

It seems that when my component rerenders and returns null, Forgo isn't correctly updating its bookkeeping. It holds onto the old detached div in args.element, which eventually makes the rerender throw an error. The component's unmount() method isn't called in this scenario, either.

The page visually renders fine, but I previously managed to get this error in an infinite loop that locked up page, and since I don't understand what's going wrong I can't just let it go.

I have a parent and a child that both depend on the same forgo-state state, and am using forgo-router.

The application flow that's happening looks like this: MyComponent onclick -> ajax -> update forgo-state -> MyComponent.render -> queueMicrotask(navigateTo('/app')), return null -> OtherComponent.render -> ajax => update forgo-state -> MyComponent.render

MyComponent shouldn't get that second render, because the page has navigated away. It should be unmounted. But because it wasn't, forgo-state tries to rerender MyComponent, but because it's holding onto the old div (even though the last render returns null) Forgo tries to rerender it and blows up.

(Why queueMicrotask? because forgo-router errors out if the very first render the app does includes an inline navigateTo() call because no components have corresponding DOM nodes yet).

If MyComponent, instead of returning null, returns something like <p>hello</p> I don't get the error. If I do the navigateTo() immediately in the render rather than in queueMicrotask, I don't get the error.

I see the error printed several times, if that means anything.

Top of render() in MyComponent:

    render() {
      if (
        authnstate.accessExpiresInHours === null ||
        authnstate.accessExpiresInHours > 0
      ) {
        queueMicrotask(() => navigateTo("/app"));
        //return <p>Redirecting...</p>;
        return null;
      }

The component is declared with bindToStates([authnstate], ...).

MyComponent lives under App, which is also bound to the same state. App contains the router:

             {...
              matchExactUrl(myComponentUrl, () => <MyComponent />) ||
             <p>Welcome!</p>}

Please let me know if I can provide any other info that'll help. I'm sorry that I can't trigger this in a minimal sandbox, only in my full application.

[jeswin]

Good find! And thanks for all the effort.

Let me try to figure out what's going on (in about 12 hours from now).
But in case I am not able to figure it out, would it be ok to do a remote/shared debug session sometime this week?

[spiffytech]

Yep, I'm up for that. I'm on US Eastern time, and I'm mostly available afternoons/evenings this week.

[jeswin]

@spiffytech Sorry, I haven't been able to look at it - been neck deep in work. It might take till friday.

I don't know what's causing this off hand - so can't point you in the right direction yet.

[spiffytech]

Sure thing. If you wind up wanting to screen share, it looks like I've got Friday morning EST open.

[jeswin]

Sorry I couldn't make time last evening - the last three months have been super hectic for me.

Looking at it today. Can you post a stack trace from when it errors?

[jeswin]

Not sure if this is the problem - but can you try the latest forgo-state please?

The following change was made:

• It does not render a node if it is disconnected. That is, node.isConnected === false.

The problem which it fixes is:

• if a parent and child are both bound to the same state and a state change is made, forgo-state will queue an update for both parent and child.
• forgo-state tracks these components (to be updated) and associated nodes internally in a list.
• In the meantime, parent removes the child component from the DOM
• When forgo-state tries to render from the tracking list above, the node is already disconnected.
• Forgo cannot work on disconnected nodes. Hence fails.

This part is a little tricky around corner cases, so I'm not sure if this will fix your problem.
So I'll wait until you try it out.

Thanks for your patience.

[spiffytech]

Using forgo-state@latest I no longer get the no-parent error. Should the isConnected check be in forgo's render pipeline? That way dependent libraries can stay blind to the internals, and future libs won't hit this same bug.

The target component's unmount callback is still not called. I do have a reproduction for that.

I've attached the stacktrace / console output for the no-child error in case it's still useful.

Thanks for looking at this.

[spiffytech]

When I was stepping through the Forge code I was trying to figure out whether the no-parent bug would go away if the component got fully unmounted, but I couldn't tell where the render was taking a wrong turn.

[jeswin]

Should the isConnected check be in forgo's render pipeline?

That's a good question, I don't know the answer - so let's discuss. forgo-state pokes with forgo's internals, and grabs internal state attached on the DOM nodes. So in that sense, it bears some responsibility for playing by forgo's rules.

Now forgo's rules (as it is currently, but can be changed) is that disconnected nodes cannot be rendered, and throws an error. We could change that behavior to not throw an error, but then users perhaps might not know why nothing is getting rendered.

In such a case, should we throw or console.log it? The downside with throwing an error are as you mentioned. The downside with only console.log is that it could lead to flows where a fundamental expection is not met (ie, component being rendered on the screen) yet the rest of the code continues executing.

The target component's unmount callback is still not called. I do have a reproduction for that.

Looking at it right away.

[jeswin]

The target component's unmount callback is still not called. I do have a reproduction for that.

You've found a bug in the implementation logic. Here's what is happening.

When a component A renders B, which renders C, which renders D, which renders a div, the component states for A,B,C and D are stored on the DIV. Now in a re-render, when the chain becomes A-B-X-Y on UL (say), C and D should be unmounted and ABXY should be attached to the UL.

The way we do that is by calling the findIndexOfFirstIncompatibleState() function, which compares ABXY (which will soon be attached to a node) with the previous chain ABCD. findIndexOfFirstIncompatibleState() will find that AB will be remounted again but C and D will not be (since they have been replaced by X and Y), and hence considers them for unmounting.

Now returning a null from Y will cause nothing to be mounted, ie - ABXY will not be mounted on anything. That is, the expectation that ABXY will be remounted soon is invalid when Y returns a null - that's the error.

Fixed in 2.1.2 and I've added two tests to cover this. But please let me know if it breaks anything.
CodeSandbox Link: https://codesandbox.io/s/forgo-child-unmount-event-forked-l09ks

For the sake of completeness of the story: a special case of this was handled in the previous code (and covered under the 'rerender may unmount parents' test) - where the final output of a rerender() was an empty node. This unmounted correctly. However, in your case the rerender() returns a div (the Router's div), but the Child component returned a null and should have been unmounted. Now the special case is removed.

[spiffytech]

I tested forgo@2.1.2 and the component now unmounts. 2.1.2 also appears to fix the no-parent issue there, even without the forgo-state update that also fixes the problem. So far I don't see any problems from the upgrade in my app.

Thanks for the explanation. I'd like to put together a CONTRIBUTING.md with architectural info and will be good to put in there.

Regarding handling the no-parent scenario, I see two scenarios where it could arise:

1. Scenarios where some consuming code should be doing cleanup. setTimeout/setInterval/websocket/forgo-state, etc. These should hook into unmount, and shouldn't hit this scenario at all if implemented properly.
2. One-off operations like click handlers where a component is unmounted while performing an async operation. Here's a reproduction of that scenario.

(2) seems like it could be common (i.e., not "exceptional"), and can't be avoided in application code without boilerplate defensive code everywhere this could happen.

If a component caught an exception from (2), I'm not sure it can do anything useful since the component is already disconnected and unmounted. Any way the component could respond to this ought to fall under (1), where the component should be doing cleanup on unmount.

The downside with only console.log is that it could lead to flows where a fundamental expection is not met (ie, component being rendered on the screen) yet the rest of the code continues executing.

If rerender after unmount is the only way to trigger this, I think it's more precise to say the expectation is the component still exists at a certain point in the application lifecycle. The concern is less about "forgo will fail to display this" and more about "you think this still exists, but it doesn't". That framing reveals that the developer misunderstands their application, and that forgo is working fine.

I've gone back and forth on what's the right way to handle it. Either an exception or a console.warn() should be equally visible in the console, and either way the component won't get displayed. So I think it comes down to whether we think this broken assumption warrants interrupting whatever the application is trying to do.

Right now, my position is that the application will be broken less if forgo doesn't interrupt a one-off async operation that calls rerender(). It's better to let that operation complete in its entirety (i.e., the call to rerender() may not be the last step in that operation). Then the developer can ignore or fix the warning at their leisure.

I do think that keeping rerender as a no-op is the right call. If we allow render() to get called, it'll cascade in to weirder issues like refs being undefined at weird times.

I can put up a PR with an improved message explaining what's happening from an application-centric perspective, and can change the exception to to a console message if appropriate.

What do you think?

[jeswin]

That's a very good point, I agree. Let's make this change then.

[jeswin]

Published 2.1.3 in which rendering a detached node is a noop.

As of now, there is no warning. If we want to warn() perhaps we should have a dev build in which warnings are visible?
Maybe we can look at this later since it mostly affects extensions.

[spiffytech]

Maybe check NODE_ENV rather than a separate build?

What's the motivation for only warning it dev?

I agree that this seems lower priority now that the underlying unmount issue is fixed.

[jeswin]

What's the motivation for only warning it dev?

Actually, I don't know if we should warn at all. If we're saying rendering disconnected nodes is a noop, then it is by design.

1. In a hypothetical case where the developer doesn't care if some of the renders are happening on disconnected nodes (say a game), it could lead to high frequency warnings.
2. The space between "all-ok" and errors is a bit confusing. It suggests something is wrong, yet continues functioning.

[spiffytech]

I could imagine rerenders of a disconnected element representing a bug in application code that would be silent if Forgo doesn't warn, but at this point I think since the current issue is solved, it's best to close the issue and revisit if it becomes an active problem. Then we'll have more information to judge the best course of action. And if it's never a problem, so be it :)