react-immutable-render-mixin icon indicating copy to clipboard operation
react-immutable-render-mixin copied to clipboard

props maybe not immutable

Open georgebbbb opened this issue 9 years ago • 6 comments

examples

<My value={Immutable.fromJS({value:123}) }  onChange={this.handleChange.bind(this)}></My>

this component's props.onChange isn't immutable so

  if (!bHasOwnProperty(keysA[i]) || !is(objA[keysA[i]], objB[keysA[i]])) {
      return false;
    }

may be miss compare mutable data like onChang

georgebbbb avatar Jan 07 '16 08:01 georgebbbb

@georgebbbb technically onChange is a property of your Component and should really be something similar to this:

const props = {
  value: 123,
  onChange: this.handleChange.bind(this),
}
<My {...Immutable.fromJS(props)} />

Update: this solution should not work since we are creating a new reference to this.handleChange every render

jurassix avatar Jan 07 '16 13:01 jurassix

Thank you but It seems that {...Immutable.fromJS(props)} does not work https://github.com/facebook/react/issues/2059 GitHub

georgebbbb avatar Jan 08 '16 02:01 georgebbbb

Yeah I can see that my example above doesn't make sense. I'll take a look this weekend and add a test case to cover this issue. Feel free to do the same. I think there is an obvious solution, I just need a test to flush it out.

jurassix avatar Jan 09 '16 01:01 jurassix

ok,thank you very much

georgebbbb avatar Jan 09 '16 10:01 georgebbbb

@georgebbbb I still haven't added any tests for this issue. But, there may be a quick fix for you.

Instead of calling bind() every render, try calling bind once, in the constructor of your Component. This should keep the onChange() handler referentially equal in child components.

class App extends React.Component {
  constructor() {
    super();
    this.handleChange = this.handleChange.bind(this);
  }
  render() {
    return (
      <My value={Immutable.fromJS({value:123}) }  onChange={this.handleChange}></My>
    );
  }
  handleChange() {
    //...
  }
}

Let me know if this fixes the issue.

jurassix avatar Feb 01 '16 00:02 jurassix

@georgebbbb were you able to confirm the above solved the issue?

jurassix avatar Feb 25 '16 14:02 jurassix