Merge initial state from new components with current state

[robertknight]

This is an initial pass at coping with the case where new state fields are added to components from one version to the next.

The approach taken here is to get the initial state of the new version of the component, given its current props and merge that with the existing state of the component. If the result is not shallow-equal to the previous state, the proxy invokes component.setState(mergedState).

The downside of this approach is that this will not always get the state to match what it would have been given the state and props transitions that have occurred since the component was instantiated.

Fixes #38

[ghost]

If I'm reading this correctly, it would help the case where you add state to a previously stateless component and have to reload the page. That would be awesome :)

I'm I correct in this understanding?

[robertknight]

Yes, indeed. See the tests for examples of exactly this.

[gaearon]

This is interesting! I'm a bit busy at the moment but I'll definitely come back to this.

[robertknight]

Thanks @Gearon , much appreciated.

[creeperyang]

It's just what I need. But maybe we don't need merge old state, just replace/overwrite?

[creeperyang]

@gaearon @robertknight I'm not sure am I right, but I'm confused about this issue/feature.

First a demo/case with react-transform-hmr:

// before
class MyComponent extends Component {
    constructor() {
       super();
       this.state = { title: 'Hi' };
    };
    render() {
        return <h1>{this.state.title}</h1>
    };
}
// then I rewrite/update the code to
class MyComponent extends Component {
    constructor() {
       super();
       this.state = { title: 'Hello' };
    };
    render() {
        return <h1>{this.state.title}</h1>
    };
}

I just want the browser to show (updated to) Hello. But currently react-proxy wont update this kind of state, and with the commit, the state wont be overwritten too.

So, is the feature I expected correct?

[ghost]

@creeperyang In the example you provided, I would expect title to remain equal to 'Hello'. If you were to update the component and add a property named 'description` with a value, I'd expect that to appear in the state object.

Basically, any new properties that are added to the state object are merged. Existing properties will retain their current values.

[creeperyang]

@danmartinez101 I agree with you. And the problem is:

1. currently, react-proxy wont update state.
2. With this commit, when this.state.title = 'hi' changed to this.state.title = 'hello'(I mean I just update code), the react-proxy wont update this.state too.

robertknight@8758e42#diff-b41c7eca2d25b7c593fb114f1d99ded4R9 with this line, the previous state property would have higher priority.

[robertknight]

@creeperyang Giving higher priority to the existing component state is what I intended to do and I think that is what @danmartinez101 meant to say, so that line of code is not wrong from my point of view. If higher priority was given to the new component's state, that would reset everything to its default values and defeat the point of react-proxy (!)

Fundamentally, the only correct way to work out what the new state should be given the new code is to replay all the events which resulted in state changes - which is what Redux dev tools do.

Merging the state is just intended to be a reasonable behavior given the lack of information about how the component got to its current state.

[creeperyang]

@robertknight Maybe you're right. But I track this issue here because I use react-transform-hmr.

As the demo above show, when I use react transfrom tool, and I change my code, I expect the state in browser sync with the code. From this point, I'm not wrong too.

But the react-proxy may not be designed to achieve this goal. or I just misunderstand the feature/function of react-transform-hmr.

[ghost]

Yea, I mixed up the values. woops

[robertknight]

I've updated the PR to call componentWillMount and componentWillUnmount when merging state to try and deal with ES2015 constructors that set up subscriptions etc.

[gaearon]

Hmm. Now that I think of it I was wrong 😞

Sure, componentWillMount fires on the server, but componentWillUnmount doesn’t. This means people tend to use componentWillUnmount to tear down whatever they set up in componentDidMount. So we can’t really call it.

At this point we should either

1. Abandon this idea
2. Embrace that constructors shouldn’t produce side effects

(2) sounds OK to me, I guess. Sorry for wasting your time! Would you mind changing it back to the way it was?

[robertknight]

1. Abandon this idea
2. Embrace that constructors shouldn’t produce side effects

I've reverted the change to call componentWillMount, componentWillUnmount.

This does show an inconsistency in React's lifecycle functions in that componentWillMount has no post-unmount counterpart which could be called on both the client and server. I wonder if it would make sense to have or is that just unnecessary complexity?

[wkwiatek]

Is this PR still valid for v3-alpha?

[robertknight]

Is this PR still valid for v3-alpha?

I honestly don't know and I'm afraid I probably won't get a chance to look at it soon. Feel free to close this one out for the moment if you're trying to tidy up open PRs.