Sync dispatch in componentDidMount() does not update component

[roadhump]

I've tried v1.0.0-alpha and noticed that this small app https://gist.github.com/roadhump/f40483704e8527f11b07 will not work if I immediately dispatch action result from componentDidUpdate but it works if dispatch async (set const async to true). It worked both ways in version 0.12.

[taylorhakes]

@gaearon I have confirmed this bug. In v1.0.0 the createConnector code

this.context.store.subscribe(::this.handleChange)

was moved from the constructor to the componentDidMount. This causes an issue with sync dispatches because the <Connector> mounts after the child components and the this.handleChange listener is not attached yet.

To fix this, we will have to move that code back to the constructor or earlier in the component lifecycle, so it will be called before the child components call dispatch.

[taylorhakes]

Here is the related issue and pull request. #178 #179

[johanneslumpe]

Wasn't this done to prevent setState etc when doing server-side rendering? @gaearon

Sent from my iPhone

On 06 Jul 2015, at 18:35, Taylor Hakes notifications@github.com wrote:

Here is the related issue and pull request. #178 #179

—
Reply to this email directly or view it on GitHub.

[taylorhakes]

Yes, I linked to the pull request and issue

[johanneslumpe]

@taylorhakes What i meant was that moving it back to the constructor or earlier in the lifecycle will possibly have negative implications for server-side rendering.

[taylorhakes]

Correct. Another option is to document that componentDidMount dispatches need to be wrapped in setTimeout or another async timer

componentDidMount() {
  setTimeout(() => {
    this.props.dispatch(/* ... */);
  }, 0);
}

That approach feels very brittle though. Currently React's mounting is synchronous, but it might change in the future.

[clearjs]

Can this behave differently depending on environment? E.g., using canUseDOM from react/lib/ExecutionEnvironment. Or am I missing something?

[johanneslumpe]

@clearjs is that exported by default? If not, it should be treated as internal file and is off limits. If it's public though, it might work.

[clearjs]

It's in the add-ons, so subject to more frequent changes. But the implementation is trivial:

var canUseDOM = !!(
  typeof window !== 'undefined' &&
  window.document &&
  window.document.createElement
); 

Obviously, this would only work in browser, not with react-native, etc.

[johanneslumpe]

I think a solution which only works on the browser is not really acceptable though.

[clearjs]

Sorry, my last comment was confusing. It does work on Node (returns false). Since for react-native server-side rendering isn't an option, this shouldn't be a problem. We'd also need to check whether the environment is react-native and behave like in browser if yes.

[clearjs]

Here's a more robust alternative, and would work with react-native as expected: https://www.npmjs.com/package/detect-node. Would need to be memoized.

[johanneslumpe]

@clearjs ok that detects node. But what about other environments like ruby? You can actually do server-side rendering there too. Or even with php. They will still execute in a JS context, but that will be V8 and not necessarily node I assume. In those cases this would still break I think.

[clearjs]

Yep, you're right.

[taylorhakes]

Another option. What about forcing the child components to mount after the Connector? Render the children only after componentDidMount on the Connector is called. No need for environment detection.

[taylorhakes]

Forgot about server not calling componentDidMount, scratch that idea.

[gaearon]

Can you please turn this into a failing test?

[gaearon]

Also, can't we do something like

    componentDidMount() {      
      this.unsubscribe = this.context.redux.subscribe(::this.handleChange);

      const nextState = this.selectState(this.props, this.context);
      if (!this.isSliceEqual(this.state.slice, nextState.slice)) {
        this.setState(nextState);
      }
    }

[taylorhakes]

I threw the example into a failing test. https://github.com/taylorhakes/redux/blob/failed-sync-dispatch/test/fullDispatch.spec.js

@gaearon I think your example will work, but I don't have time to test it right now.

[gaearon]

Should be fixed in #195 via 406ca62.