forceUpdate warning if component is unmounted by componentWillReact

[timdorr]

I was implementing some auth logic in my app and ran into some of the wonderful forceUpdate errors I'm sure you hate at this point.

The cause of this was my checking for authorization changes within componentWillReact, looking to see if I was now logged out/in and redirecting to/from the login screen. Basically, I had something like this in on my top-level App wrapper component:

componentWillReact() {
  if (!this.props.auth.loggedIn)
    this.context.router.push({ pathname: '/login' })
}

(BTW, I'm using the new Provider component for this. Super helpful!)

This created a side effect of having my redirect unmount the App component (It will switch to another top-level tree with my Login component). This side effect isn't checked for the forceUpdate call directly after the lifecycle method runs.

I haven't checked out the project and set things up to run tests, otherwise this would be a PR. But I believe this will fix the issue:

if (typeof self.componentWillReact === "function")
  self.componentWillReact();
if (!self._calledComponentWillUnmount)
  React.Component.prototype.forceUpdate.call(self)

I don't know how stable the _calledComponentWillUnmount property will be over time. It was only added in 15.0.2, so you probably want to DIY for older versions. You can do something similar to #44, but only tracking if componentDidUnmount was called.

[mweststrate]

Hi Tim,

Version 3.5.0 should solve this issue, it checks for unmounted state first before calling forceUpdate.

[timdorr]

Yep, that did it! Thanks for looking into and fixing this, @mweststrate!

[mweststrate]

@timdorr there is a regression bug with this change. would you mind testing whether either mobx-react@3.5.0-fix85 or mobx-react@3.5.0-fix85-2 also fixes the issue? regression: #85

[mweststrate]

Hmm actually I am wondering whether componentWillReact should be allowed to have side effects in the first place. It is kinda part of the render lifecycle itself. Would an autorun / when not be a nicer solution? That doesn't even need to be in a component at all. Or it could be setup in componentWillMount which feels like a more appropriate place:.

componentDidMount() {
  when(
    () => this.props.auth.loggedIn === false,
    () => this.context.router.push({ pathname: '/login' })
  )
}

This no longer changes the routing as side effect of a rendering, but instead as a side effect of changing the loggedIn state, which feels conceptually more correct. I think even that in the current setup your app would not redirect if your component doesn't render any observable data, because the component would not react to anything in that case. So probably instead of preventing the React warning, I should maybe add a warning when a side effect in componentWillReact is detected. What do you think?

[timdorr]

Ah, I didn't know about when. Still learning the API surface.

The only reason I didn't use autorun/when is I don't know how to cancel that observer when my component unmounts. It's similar to one of my dropdown components, where I start to watch for document clicks when the component mounts and remove that listener when it unmounts. Can you do cleanup of these listeners?

[mweststrate]

yes both return a function to clean up

Op do 14 jul. 2016 17:23 schreef Tim Dorr notifications@github.com:

Ah, I didn't know about when. Still learning the API surface.

The only reason I didn't use autorun/when is I don't know how to cancel
that observer when my component unmounts. It's similar to one of my
dropdown components, where I start to watch for document clicks when the
component mounts and remove that listener when it unmounts. Can you do
cleanup of these listeners?

—
You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub
#73 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABvGhElOVkvx8Cq6nJ71kT1yL7O4aDksks5qVlSMgaJpZM4JCrG4
.

[timdorr]

That's what wasn't clear. I'll see about a PR for some docs improvements around that.

In that case, I would treat componentWillReact like componentWillUpdate and require any side effects/state changes to be avoided there.