Should the Components really extend PureComponent?

[edorivai]

Hey, really awesome library, thanks so much for sharing!

tl;dr

I think that all Components that use render props should extend Component, instead of PureComponent.

Longer version

My colleague noticed that the <Form> component is a PureComponent, so he thought it'd be a good idea to maintain referential identity between renders, and define the render function in such a way that it would allow <Form> to opt-out of rerendering. Like so:

class Form2 extends Component {
  renderForm = ({ handleSubmit }) => {
    ...fancy rendering stuff
  };

  render() {
    return <Form render={this.renderForm} />;
  }
}

This works fine, as long as renderForm is a pure function, solely depending on it's own parameters. However, as soon as you mix in state or props from the wrapping component (Form2 in the example), the renderForm function passed to <Form> will become stale.

This is all much better explained in the excellent article by @ryanflorence: https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

I think the quote most relevant for this issue is the following:

That means an inline render prop function won’t cause problems with shouldComponentUpdate: It can’t ever know enough to be a PureComponent.

As I understand it, if you use render props, you should never extend PureComponent, since you don't know what's going on inside the render function.

What are your thoughts?

[klis87]

@edorivai You can pass this external value as a prop to wrapped Form, then it will rerender as its prop would change. Changing Form, Field components to be not pure components would degrade performance in many cases, I really think PureComponent is a good default.

You could also use component instead of render, then you would need to pass those external props anyway so they could be available in passed component

[edorivai]

You can pass this external value as a prop to wrapped Form

Yeah, nice hack. I guess that would work.

Changing Form, Field components to be not pure components would degrade performance in many cases

I understand what you're saying, though I also agree with Ryan's standpoint, that PureComponent does not strictly improve performance; that really depends on the structure of the app. And one cannot really claim anything without measurement.

Moreover; as soon as someone passes the render function inline, this completely nullifies the effect of PureComponent, since inline functions break referential identity. I would expect most people to define the render prop inline, as this is also how I've seen it described in the docs. And in case of an inline render function, Component will be strictly faster than PureComponent.

On the other hand, I agree it would be good to enable consumers of this lib to control whether <Form> is a PureComponent or not. However, I realize this is easier said than done.

I really think PureComponent is a good default

Not sure if I agree on this, as it could lead to confusing behavior if you're not 100% aware of what you're doing. Perhaps it could be preferred to have the more predictable, but less performant behavior of Component by default, and allow for switching to a (potentially) more performant solution with PureComponent through opt-in. Given that PureComponent in combination with render props has some edge-cases, I fear that people with less React knowledge will burn themselves.

[klis87]

@edorivai good point with inlined render function, indeed PureComponent is always slower than Component without any benefit in this case.

I think you started a good discussion :)

[edorivai]

For what it's worth, I asked around on twitter. Seems like it's really preferred not to extend PureComponent when using render props.

Thread: https://twitter.com/EdoRivai/status/961930580702253056

Also, as quoted from the official React docs:

In cases where you cannot bind the instance method ahead of time in the constructor (e.g. because you need to close over the component’s props and/or state) should extend React.Component instead.

[erikras]

Okay. You've made a convincing argument. 👍

[erikras]

Published fix in v3.1.1.

[bertho-zero]

I struggled a lot of time on the optimization of a large form, It should be stipulated somewhere in the doc that it is better to declare render render outside the render method, in a component or a instance method.

https://reactjs.org/docs/render-props.html#caveats

Also, because subscription props are (often) created in the render method, they always have a different reference and never pass the shallowEqual.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.