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

domProps props on DOM elements #34

Closed
Cap32 opened this issue Mar 29, 2017 · 1 comment
Closed

domProps props on DOM elements #34

Cap32 opened this issue Mar 29, 2017 · 1 comment

Comments

@Cap32
Copy link

Cap32 commented Mar 29, 2017

According to Spreading props on DOM elements, it is recommended to use a domProps object to spread props on DOM elements.

GOOD

By creating props specifically for DOM attribute, we can safely spread.

const Sample = () => (<Spread flag={true} domProps={{className: "content"}}/>);
const Spread = (props) => (<div {...props.domProps}>Test</div>);

But directly passes domProps prop may cause unnecessary update in PureComponent.

Here is an example

import React, { Component, PureComponent } from 'react';
import { render } from 'react-dom';

class Spread extends PureComponent {
  componentDidUpdate() {
    console.log('updated'); // will show
  }

  render() {
    return (
      <div {...this.props.domProps}>Test</div>
    );
  }
}

class App extends Component {
  componentDidMount() {
    this.forceUpdate();
  }

  render() {
    return (
      <Spread flag={true} domProps={{ className: 'content' }}/>
    );
  }
}

render(<App />, document.getElementById('mount'));

Here is my solution by omitting unknown HTML props

import React, { Component, PureComponent } from 'react';
import { render } from 'react-dom';

class Spread extends PureComponent {
  componentDidUpdate() {
    console.log('updated'); // will NOT show
  }

  render() {
    const {
      flag, // eslint-disable-line
      ...other,
    } = this.props;
    return (
      <div {...other}>Test</div>
    );
  }
}

class App extends Component {
  componentDidMount() {
    this.forceUpdate();
  }

  render() {
    return (
      <Spread flag={true} className="content"/>
    );
  }
}

render(<App />, document.getElementById('mount'));

Another solution is caching domProps object, but I think both of my solutions are not elegant 😂

@vasanthk
Copy link
Owner

vasanthk commented Apr 16, 2017

@Cap32 This is a valid example to look into.
PureComponent only does shallow checks, hence the re-render.

As per docs:

React.PureComponent's shouldComponentUpdate() only shallowly compares the objects. If these contain complex data structures, it may produce false-negatives for deeper differences. Only extend PureComponent when you expect to have simple props and state, or use forceUpdate() when you know deep data structures have changed. Or, consider using immutable objects to facilitate fast comparisons of nested data.

Furthermore, React.PureComponent's shouldComponentUpdate() skips prop updates for the whole component subtree. Make sure all the children components are also "pure".

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

No branches or pull requests

2 participants