Why not just call trySubscribe in componentWillMount if typeof window !== 'undefined'?

[faceyspacey]

I'm referring to this line in connectedAdvanced:

https://github.com/reactjs/react-redux/blob/master/src/components/connectAdvanced.js#L154

It seems it would make things faster for the case where child components trigger a state change, and perhaps even a little bit for components that don't.

I understand the correct idiom is to define state in your constructor or as an instance property, but still. Are we simply trying to prepare for a very rare use case that window is defined on the server. When running tests in Jests, it will be defined. But if componentWillUnmount is called there, it's not an issue there as well. And if it is, perhaps we can detect jest and do the componentDidMount technique currently in use.

Why is typeof window !== 'undefined' not considered a good enough solution?

[timdorr]

Because componentWillMount does this for us. We don't really care about it being in the browser or the server or whatever. We care about whether there will be a cleanup stage when the component is unmounted. When rendering to a string, there is no unmount step, so you can get a memory leak very easily. Doing an environment check doesn't tell us that, so it's a weak smoke test to be doing that sort of thing.

[faceyspacey]

I know the memory leak thing. I read the comment. The "weak smoke test" thing was what I was assuming, but it's the use case 99% of the time. So performance is being sacrificed for a 1% of the time case. Possibly a 0% of the time case.

[faceyspacey]

where is there a window object when componentWillUnmount is not called?

[gaearon]

Can you expand a bit more on the speed benefit we'd get if we didn't do this? I don't fully understand.

[timdorr]

where is there a window object when componentWillUnmount is not called?

If you renderToString in the browser, which is a perfectly valid thing to do.

[faceyspacey]

That's a valid use case. No other way to detect we are running in that function?

The perf benefit is forceUpdate doesn't need to be called in the function I linked above because the selector will get the necessary state before the first render. So one render vs 2.

[gaearon]

Calling forceUpdate in componentDidMount is really unfortunate if that's what happens. I haven't been keeping a close look at v5 and I'm worried there's a bunch of things that may become worse in future versions of React.

I think at this point we should wait for React 16 to be out and then re-evaluate choices made in v5 with regards to how subscriptions are set up and updates are scheduled. It's probably not worth doing that now though.

[gaearon]

In the meantime you can keep use v4 which to my knowledge doesn't do this. I might be wrong though or misunderstand this discussion. Sorry if this is the case.

[faceyspacey]

To be clear, forceUpdate is only called if child components (not just the wrapped component but the entire branch) dispatch an action resulting in a state change.

[jimbolla]

@gaearon This is behavior that existed in v4.

[faceyspacey]

It seems to me renderToString should set some indicator that componentWillUnmount will not be called. Simple as that. It's important enough that it's worth it to find a way. Otherwise ur flying blind and end up with hacks like this. Client code deserves to know if componentWillUnmount will not be called.

Redux is the second most important package in the React world. Can that not be coordinated? Perhaps surrounding the 16 release.

[markerikson]

@faceyspacey : I'm kind of confused. What's the original concern here? A performance issue? If so, what's the actual perf impact in play, and why is it such a big deal?

[markerikson]

@gaearon : I know React 16 is still a ways off, but can you point to any specific concerns about v5's behavior and implementation? I know you've expressed some hesitation in a couple different threads, but I'm really not clear on what actual things are bothering you.

[faceyspacey]

forceUpdate can possibly be called here:

https://github.com/reactjs/react-redux/blob/master/src/components/connectAdvanced.js#L156

causing a 2nd unnecessary render after mounting.

This happens when actions are dispatched in componentWillMount of the wrapped child component. Or if any children dispatch. In either case the whole branch is subject to another render.

[markerikson]

So, you're specifically worried about one potential extra render?

FWIW, I don't think the phrase "the whole branch is subject to another render" is 100% right. Even if this parent component does re-render, connected descendants would likely see that there were no changes and skip re-rendering (I'm assuming there's at least one connected descendant based on the scenario, although I guess a child could be calling this.props.someActionCreator() in cDM ).

[faceyspacey]

Absolutely.

Jank during animations coinciding with renders is a constant problem with React on real apps.

[markerikson]

Do you have any specific benchmarks indicating that that line is an actual issue?

[timdorr]

Why would that ever fire? We run the selector via the constructor, so nextProps should equal selector.props and selector.shouldComponentUpdate should still be false.

Question I have: Why run the selector twice? I realize it's memoized, but we've got two problems: 1) You're doing some minor work twice (no biggie) 2) render() isn't pure (biggie!)

Why not just initialize the stateful selector in makeSelectorStateful with those properties already defined and set for the initial state? That would seem to obviate the need for the forceUpdate() call as well.

[faceyspacey]

@markerikson No, but of course it is. Frames dropped is a function of the number of simultaneous animations + components re-rendering. It's not black or white, there's a gradient to it. When apps go from being "sites" to animation-heavy apps, you're dropping frames all the time. Less so in RN, but especially within the browser. And no matter how good Fiber is, this will always be a problem in some apps. As a result, key open source tools like react-redux need to perform at their absolute best, no expenses spared.

@timdorr if it's never called, forceUpdate should be removed. But that's not the case.

The following seems to be the reason why this is done (December 19, 2015):

#210 (comment)

By the time componentWillMount fires on the child, it is too late to change the props child receives. Since store dispatch changes child's props, this must happen on the next render.

However since SSR does not have componentDidMount, we have this problem:
during SSR, you can't dispatch anything in componentWillMount and expect it to be reflected in the wrapped component or any children components it passes derived props to! That's a major problem, though children components that directly connect to the store should be fine.

I think we need to solve another way what @gaearon mentioned regarding it to being "too late to change the props children receive" and then after change the trySubscribe implementation to:

componentWillMount() {
  if (!shouldHandleStateChanges) return

  if (React.hasComponentWillUnmount()) {
    this.subscription.trySubscribe()
  }
}
// no more componentDidMount

[faceyspacey]

solution: a componentWillMount callback you can pass to connect. That way we don't have the problem of the wrapped component rendering with outdated props, and we have an idiomatic way to recommend to users.

Even if we got this working via that solution, the one caveat is if in componentWillMount you dispatch something that changes what parents or siblings render, they won't render the correct thing. It's too late. Damn. That was clearly @gaearon 's initial reasoning here.

This makes double renders unavoidable, and therefore it doesn't really matter if we do trySubscribe in componentDidMount since we need the method anyway.

I actually never dispatch in componentWillMount, which I think is the correct idiomatic way (i.e. SSR involves rendering correctly once from pre-hyrdated state). But I ran into this problem when a developer I was talking to last week couldn't figure out why SSR wasn't working correctly. At the very least this should be mentioned in bold as a DONT DO in the readme.

As far as trySubscribe goes, it turns out its not the real problem here and it's fine.

[faceyspacey]

SSR is more important than ever. With simultaneous SSR + Code-Splitting being a generally solved a problem without having to use Next.js, we should expect all apps to have SSR. The SPA days are over.

Though we may not be able to avoid the double rendering, we should find a way for dispatching to reliably happen in componentWillMount so SSR isn't broken. I.e. we have to find a way to double-render during SSR. smh.

React needs componentDidMountDuringRenderToString specifically for cases like this. I see no other way besides a complete re-render.

[faceyspacey]

And by the way, I think the correct and only solution was already arrived at. Maybe Fiber will allow for an additional opportunity to re-render during SSR. Here's a PR to the readme:

#726

The case is likely closed until Fiber. @gaearon if you could bring this up with the Fiber team that would be awesome. Talk to you all later.