ReactCurrentOwner set before constructor call causes false warnings

[gaearon]

ReactCurrentOwner is set before the constructor call. If a constructor has side effects like a Flux Store dispatch, then other components may get their setState() called. They will log a warning that setState() cannot be called from render().

This warning is misleading because this is not what happened. It protects against setState() calls from render() but what really happens is one component causing a side effect in its constructor, leading to other component getting its setState() called, which shouldn't result in a warning.

I'm not sure it's worth fixing but something #5302 reminded me about.
Original issue: reduxjs/react-redux#129 (comment).

Warning message:

Warning: setState(...): Cannot update during an existing state transition (such as within render). Render methods should be a pure function of props and state.

Per reduxjs/react-redux#129 (comment), the issue is new to 0.14.

[jimfb]

Good first bug, diff should include a unit test to verify we don't regress this again in the future.

[gaearon]

What's the correct behavior? Warn with a different message? Don't warn at all? Can we even call setState() from the constructor?

[jimfb]

Currently, I think setState is ignored (ie. a no-op) if invoked during a constructor. Assuming we keep the same behavior, we would just display a different error message.

Tracking "what is the correct behavior" in #5342.

[acdlite]

It looks like there are a few places where React interprets ReactCurrentOwner.current == null to mean that a render method is currently executing. That's not quite true, though, because as @gaearon points out, the current owner is also set at component initialization.

Would a good way to solve this be to add a separate module called ReactCurrentRender (or something) that keeps track of whether a render method is currently being executed?

[jimfb]

@acdlite Not too many, I hope. We're trying to get rid of owner in the core.

Yeah, a ReactCurrentRender module sounds like as good an approach as any. I don't think it actually makes the owner situation any worse. cc @spicyj for ideas.

[sebmarkbage]

I have a todo list document outlining all the uses of ReactCurrentOwner and alternatives that I want to replace it with. Basically, isCurrentlyRendering that is only set in __DEV__ because they're all used for warnings.

[sebmarkbage]

ReactCurrentOwner, and subsequently _owner, should only be used for string refs and nothing else. When we eventually deprecate string refs, it will be gone completely.

[acdlite]

I believe this can be closed now #5406

[sophiebits]

Thanks! (Tip: If you write "Fixes #5313." in your commit message then the issue gets closed automatically when the PR is merged.)