React v16: Error during mount causes instance to never be removed from mountedInstances

[tyscorp]

I'm guessing componentWillUnmount isn't called. Would a fix be to listen to componentDidCatch, splice the array as in componentWillUnmount and then rethrow the error?

[lourd]

Hey @tyscorp, thanks for the report. That sounds like a practical solution, from what I understand so far. Do you think you could reproduce what you're seeing in a test and open a PR with it to start work?

[tyscorp]

So, I investigated fixing this and I'm not sure how to solve this in a nice way. I did come up with a hacky solution but it will not work nicely with React's upcomingAsyncMode.

I tried the componentDidCatch idea but it caused any error to look like it was coming from react-side-effect and not the actual component that had an error which made debugging annoying.

Moving the code to componentDidMount fixes issues with errors but the order of the mounted instances are incorrect and inconsistent.

The "working" hacky solution works by splitting the logic from componentWillMount into a two phases. When the component mounts it is added to the mountedInstances array but marked as pending. A 0ms timer with a callback to remove the component is also set up. Then in componentDidMount the component is marked asresolved, the timer is cleared and emitChange is called. emitChange is changed to only reduce props for components that are in the resolved state.

The timeout is necessary because as far as I know there is no way of telling if the component failed to mount without implementing componentDidCatch and losing the error's context. I think the same approach could work in AsyncMode but the timeout would have to be longer. (1 second?)

It would be cool if we could get input from some of the React core devs as they will have more insight into how to solve this in a nicer way.

[lourd]

Thanks for your exploratory work on this Tyson. Agreed, this sounds tricky.

@gaearon, could you weigh in on this? How should react-side-effect handle errors that occur when a component is mounted?

[gaearon]

Issues like this is why we're moving towards deprecating componentWillMount in the future.

I think it's a bit shady that we currently track instances. This seems more like a use case for context. This also exposes a flaw in the model itself: it doesn't consider that there can be siblings. So the stack abstraction is wrong in the first place:

<A>
  <B>
    <C> // should this win?
  </B>
  <D /> // or this?
</A>

I guess this API is still useful though. Although if it went through a major change that wouldn't be a big deal because its use is pretty isolated. So maybe we can come up with something better right away instead of trying to patch up a broken model.

The key thing about async changes (and that relates to error boundaries) is that the rendering phase (componentWillMount, render) can be paused, resumed, aborted, restarted from scratch, etc. Only the commit phase (componentDidMount, componentDidUpdate, componentWillUnmount) always happens when we're sure what's going on. Any side effects should be done in the commit phase. They are done in the commit phase now too, but the guarantees about componentWillMount order are no longer truly there which is why we have this issue.

I'm not sure how to solve this yet. But we'll need to figure this out. It may be representative of a broader pattern people might be relying on in another projects too.

[gaearon]

I asked @bvaughn to take a look since he's thought about these hooks for longer than me. But if either of us forgets please ping us again and I'll come back to this.

[lourd]

Will do, thank you Dan

[bvaughn]

Generally, as has been pointed out above, the safe practice here is to use componentDidMount and componentDidUpdate lifecycles for side effects and mutations.

I'm probably overlooking something really stupid here, but could you not just change these lines:

componentWillMount() {
  mountedInstances.push(this);
  emitChange();
}

To this:

componentDidMount() {
  mountedInstances.unshift(this);
  emitChange();
}

This way it's done at a safe time (mount) and it supports the deepest-child-wins behavior that libs like gaearon/react-document-title rely on?

FWIW, this would reverse the order of the items tested in "should collect props from all instances", but it's not obvious to me if this is actually a problem. (Is the order significant or just the fact that all props are collected.)

[bvaughn]

As Dan pointed out (out of band) this lib supports SSR too, so the above suggestion would not work for that case.

The SSR-only static instanceMounted lifecycle proposed via reactjs/rfcs/pull/8 would be helpful for that case (although it would not be a backwards compatible solution).

[gaearon]

Since we already branch on whether we have the DOM, it seems acceptable to use the same check to push instances in render (if we're rendering on SSR).

[tyscorp]

@bvaughn making that change works in some cases but not others for me. It always seems to work on the initial ReactDOM.render, but in some places with subsequent updates it doesn't work whereas componentWillMount pushes in the correct order for all my usecases.

[bvaughn]

I had written a version of that locally, Dan- (using will-mount or did-mount based on canUseDOM). Seems a reasonable fallback solution.

It always seems to work on the initial ReactDOM.render, but in some places with subsequent updates it doesn't work whereas componentWillMount pushes in the correct order for all my usecases.

Can you elaborate on these cases, @tyscorp?

[bvaughn]

By the way, I pushed the changes I was playing with (and some new tests I added) to PR #45 in case it's useful. It's a little sloppy. I wasn't intending to push it.

[tyscorp]

There are a few cases where I have no idea what is happening, but I can repro the issue pretty consistently by wrapping a component that uses react-side-effect in react-loadable and webpack's import. The components mount in reverse order the first time, then correctly afterwards.