Skip to content
This repository has been archived by the owner on May 4, 2020. It is now read-only.

props maybe not immutable #10

Open
georgebbbb opened this issue Jan 7, 2016 · 6 comments
Open

props maybe not immutable #10

georgebbbb opened this issue Jan 7, 2016 · 6 comments
Labels

Comments

@georgebbbb
Copy link

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

@jurassix
Copy link
Owner

jurassix commented Jan 7, 2016

@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

@georgebbbb
Copy link
Author

Thank you
but It seems that {...Immutable.fromJS(props)} does not work
facebook/react#2059
GitHub

@jurassix
Copy link
Owner

jurassix commented Jan 9, 2016

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.

@georgebbbb
Copy link
Author

ok,thank you very much

@jurassix
Copy link
Owner

jurassix commented Feb 1, 2016

@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
Copy link
Owner

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

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

No branches or pull requests

2 participants