Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getInitialState -> componentWillMount order is not preserved #8

Closed
endragor opened this issue Apr 19, 2015 · 5 comments · Fixed by #10
Closed

getInitialState -> componentWillMount order is not preserved #8

endragor opened this issue Apr 19, 2015 · 5 comments · Fixed by #10
Labels

Comments

@endragor
Copy link

react-mixin makes mixin's getInitialState wrapped into componentWillMount and called after the mixin's original componentWillMount.
Normally, state is initialized from getInitialState before componentWillMount is called, and so componentWillMount is able to rely on the initialized state.

As an example, this breaks Formsy.Mixin that uses this.state from componentWillMount.

@brigand brigand added the bug label Apr 19, 2015
@brigand
Copy link
Owner

brigand commented Apr 19, 2015

Doing it in componentWillMount is necessary because getInitialState just doesn't exist, and we can't augment the constructor.

I think the fix here is manually merging state in componentWillMount rather than using setState, and doing it before calling the actual componentWillMount functions.

A PR would be very appreciated; it's going to be a little while before I have time to do this and fix up the tests.

@brigand
Copy link
Owner

brigand commented Apr 19, 2015

@endragor does 1.2.1 work for you?

@endragor
Copy link
Author

Right now - no, it doesn't, because of bug in React: facebook/react#1740
Changing this.setState(getInitialState.call(this)) to this.state = merge(this.state, getInitialState.call(this)) made it working (merge is self-written).

@brigand
Copy link
Owner

brigand commented Apr 20, 2015

@endragor I think it's fixed.

import React from 'react';
import reactMixin from 'react-mixin';

var Mixin = {
  getInitialState(){
    return {'foo': 'bar'};
  },
  componentWillMount(){
    console.log(JSON.stringify(this.state, null, 4));
  }
}

class C extends React.Component {
  constructor(){
    this.state = {'baz': 'quux'};
  }
  render(){
    return <div />;
  }
}

reactMixin.onClass(C, Mixin);

React.renderToString(<C />);

// stdout
{
    "baz": "quux",
    "foo": "bar"
}

The only concern is how duplicate keys are handled, I don't think it's done correctly currently.

@endragor
Copy link
Author

@brigand thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants