search

FormidableLabs / react-fast-compare

fastest deep equal comparison for React
MIT License
1.59k stars 54 forks source link

Comparing DOM elements #33

Closed viper1104 closed 5 years ago

viper1104 commented 6 years ago

Comparing two DOM elements always returns true. Need to add a comparison of DOM elements.

chrisbolin commented 6 years ago

Thanks for the contribution, @viper1104!

I really appreciate the tests, too! I think I'd like to change a bit of the behavior tho:

  const element1 = document.createElement('div');
  const element2 = document.createElement('div');

  equal(element1, element2); // returns false with this PR

I'd like equal(element1, element2) to return true. Think of equal(document.createElement('div'), document.createElement('div')) as similar to equal({key: "a"}, {key: "a"}); they are different objects, but they have identical structures.

viper1104 commented 6 years ago

I'm not sure if this is correct for the react. Elements can have the same class and structure, but they are completely different. And then the component will not be re-rendered. For example:

class PortalComponent extends Component {
  shouldComponentUpdate(nextProps, nextState) {
    return !deepEqual(this.props, nextProps);
  }

  render() {
    return ReactDOM.createPortal(this.props.children, this.props.target);
  }
}

class Container extends Component {
  render() {
    return (
      <div className="container">
        <PortalComponent target={`${document.querySelector(this.someCondition() ? '.block-1' : '.block-2'} .portals`)}>
      </div>
    );
  }
}

<!DOCTYPE html>
<html>
  <head></head>
  <body>
  <div class="block-1">
    <div class="portals"></div>
  </div>
  <div class="block-2">
    <div class="portals"></div>
  </div>
  </body>
</html>
chrisbolin commented 6 years ago

@viper1104 i'll look into this more. i think there is a way to satisfy your concern and still remove needless re-renders. Thank you for the specific example, that's really helpful!

xaviergonz commented 5 years ago

any update into this?

boygirl commented 5 years ago

@chrisbolin I'm curious about this change as well and how it might impact Victory which makes extensive use of cloneElement.

chrisbolin commented 5 years ago

There's probably some confusion on this issue (I know I was confused for a while) between using this library to compare DOM Elements and React Elements.

This PR only targets DOM Elements. Handling of React Elements goes unchanged (so Victory won't be changed, @boygirl). I want to make some changes to the tests, but after I do this should be merged.

To be safe, we could have a beta version for testing if you all would like.

chrisbolin commented 5 years ago

Merged this. will release after...

chrisbolin commented 5 years ago

released in react-fast-compare@2.0.3 👍