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

Add React.PureComponent, inherit purity for functional components #6914

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@sophiebits
Member

sophiebits commented May 28, 2016

React.PureComponent

This provides an easy way to indicate that components should only rerender when given new props, like PureRenderMixin. If you rely on mutation in your React components, you can continue to use React.Component.

Inheriting from React.PureComponent indicates to React that your component doesn't need to rerender when the props are unchanged. We'll compare the old and new props before each render and short-circuit if they're unchanged. It's like an automatic shouldComponentUpdate, but it also affects the behavior of functional children that are rendered:

Functional components

We've heard clearly that most React users intend for their functional components to be pure and to produce different output only if the component's props have changed (https://mobile.twitter.com/reactjs/status/736412808372314114). However, a significant fraction of users still rely on mutation in their apps; when used with mutation, comparing props on each render could lead to components not updating even when the data is changed.

Therefore, we're changing functional components to behave as pure when they're used inside a React.PureComponent but to rerender unconditionally when contained in a React.Component:

class Post extends React.PureComponent {  // or React.Component
  render() {
    return (
      <div className="post">
        <PostHeader model={this.props.model} />
        <PostBody model={this.props.model} />
      </div>
    );
  }
}

function PostHeader(props) {
  // ...
}

function PostBody(props) {
  // ...
}

In this example, the functional components PostHeader and PostBody will be treated as pure because they're rendered by a pure parent component (Post). If our app used mutable models instead, Post should extend React.Component, which would cause PostHeader and PostBody to rerender whenever Post does, even if the model object is the same.

We anticipate that this behavior will work well in real-world apps: if you use immutable data, your class-based components can extend React.PureComponent and your functional components will be pure too; if you use mutable data, your class-based components will extend React.Component and your functional components will update accordingly.

In the future, we might adjust these heuristics to improve performance. For example, we might do runtime detection of components like

function FancyButton(props) {
  return <Button style="fancy" text={props.text} />;
}

and optimize them to "inline" the child Button component and call it immediately, so that React doesn't need to store the props for Button nor allocate a backing instance for it -- causing less work to be performed and reducing GC pressure.

Add React.PureComponent
This provides an easy way to indicate that components should only rerender when given new props, like PureRenderMixin. If you rely on mutation in your React components, you can continue to use `React.Component`.

Inheriting from `React.PureComponent` indicates to React that your component doesn't need to rerender when the props are unchanged. We'll compare the old and new props before each render and short-circuit if they're unchanged. It's like an automatic shouldComponentUpdate.
@nfcampos

This comment has been minimized.

Contributor

nfcampos commented May 29, 2016

Two questions:

  • for an app that consists entirely of functional components to opt in to pure functional components the root component needs to be a class component (which extends PureComponent), it can't be a functional component itself, right?
  • presumably every library that renders user-provided components — eg. react-router, react-redux, etc. — will need to provide two versions of whatever internal component renders the user provided components, one that extends PureComponent and one which extends Component so that library users can be offered the choice of having their functional components further down the tree be pure or not. I guess this is not really a question, more of a realisation that it looks like this will end up being an option in the api of a majority of libraries once this gets released.
@nfcampos

This comment has been minimized.

Contributor

nfcampos commented May 29, 2016

I guess what I mean to say is this: this would definitely be a great idea if the creator of a component was also always in control of the parent of the component he created, since this is not true (eg. react-router is the one who renders your route components for you, not you), it might still be a great idea, but it is less so because it will effectively force every library that renders user components to expand its API so that users can get back control over how their components behave

@timdorr

This comment has been minimized.

timdorr commented May 29, 2016

@nfcampos In react-router's case, given that we are pretty much always at the root of the render tree, having us be PureComponents won't really make much sense. I also don't think it's common to have functional components as your route components. I'm normally an advocate of our users setting up a dedicated route component that delegates any app activity to components above it. It serves as a good handing off point for route information and keeps separate of responsibilities high.

Edit: Also, react-redux does some really neat tricks to cache the rendered element and update only when things have actually changed. It's pretty neat and would eliminate some of the need to have a separate version that extends PureComponent.

Is there any support here for React.createClass components? Via a toggle property of some sort?

What would be interesting is the ability to swap out "pureness" at runtime, depending on certain conditions (for example, when an app might be trickling in a lot of data that doesn't get batched up). It looks like this is achievable via this.isPureReactComponent, but is this intended to be a public API?

@syranide

This comment has been minimized.

Contributor

syranide commented May 29, 2016

Intuitively I kind of agree with @nfcampos on this. It seems to me that pureness isn't primarily a trait of the component, but the caller/usage of the component. It seems that you would have to expose one non-pure and one pure of every third-party component and that may even be complicated in more complex cases where there is a larger hierarchy being rendered internally.

Also, I'm curious, what's the behavior of <PureComponent><MyComponent /></PureComponent>, when elements are rendered as children, do they inherit pureness from the parent or the owner?

@satya164

This comment has been minimized.

satya164 commented May 29, 2016

Agree with @nfcampos. IMO it's better and easier to let the component decide if it's pure or not instead of the parent component doing it. In the long run, it might create confusion and unintended behaviour.

@satya164

This comment has been minimized.

satya164 commented May 29, 2016

I think something like MyComponent.shouldComponentUpdate = () => true or MyComponent.isPure = true can work.

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

for an app that consists entirely of functional components to opt in to pure functional components the root component needs to be a class component (which extends PureComponent), it can't be a functional component itself, right?

This is correct.

presumably every library that renders user-provided components — eg. react-router, react-redux, etc. — will need to provide two versions of whatever internal component renders the user provided components, one that extends PureComponent and one which extends Component so that library users can be offered the choice of having their functional components further down the tree be pure or not. I guess this is not really a question, more of a realisation that it looks like this will end up being an option in the api of a majority of libraries once this gets released.

In some cases, but not necessarily. This is not much different from today: libraries have to take a stance on where they are on the mutability spectrum. For example React Redux already has pure class containers, so it will switch to PureComponent. (But it already provides pure: false opt-out, so I guess we’ll have to keep offering that options.)

As for other libraries, it’s no different then the situation today. If you don’t want to force your users to be immutable, you can just export a regular class. If the consumer wants optimizations, they can wrap the children into their pure container:

<Router> // not optimized
  <MyApp> // my own, optimized! 
    <Header /> // functional, optimized thanks to MyApp

So I think

It seems that you would have to expose one non-pure and one pure of every third-party component

is unnecessary overkill, and doesn’t need to happen. If you’re not sure your users are immutable, just provide Component. It’s no different from choosing whether to provide shouldComponentUpdate() today—it’s exactly the same decision third party component have been doing for a long time.

I expect that most libraries will provide just Components, and if you want to opt into the optimizations, you just wrap your components in a PureComponent.

The optimizations kick in if the closest class parent is a PureComponent so, e.g. in case of React Router, you’d only need to make your App top-level handler a PureComponent, for the rest of the app to work.

In any case, there should be no goal of “making every component pure”. It’s just an optimization that would cover many cases, and generally benefit apps that use immutability. You shouldn’t be chasing that optimization with every component in your app—React reserves the right to not enable it in some cases anyway.

@glenjamin

This comment has been minimized.

Contributor

glenjamin commented May 29, 2016

It's worth noting that in JS having the same arguments isn't enough to ensure purity.

function Since(props) {
return <p>{new Date() - props.date}</p>;
}

Treating a component like this as pure because of some property of the parent would not work.

I think for functional components to be marked as pure it'd have to be opt-in or opt-out per component, and for back-compat reasons that probably means opt-in.

this._context,
// Element updates are enqueued only at the top level, which we consider
// impure
false

This comment has been minimized.

@gaearon

gaearon May 29, 2016

Member

Maybe isParentPure? I didn’t realize it’s a boolean until this far.

This comment has been minimized.

@sophiebits
@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

@glenjamin If this worked before, it only worked because some component above it caused it to re-render by setState or forceUpdate. Since this function wouldn’t have PureComponents above it, the existing pattern wouldn’t break, so I don’t see why this is breaking compatibility.

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

To clarify again: this change doesn’t make all functional components pure. You still have to opt in by making their closest class parent a PureComponent. At this point we know that all children would be effectively pure anyway, because if the parent didn’t rerender, they wouldn’t either. This is why it’s safe to make them as pure in this situation.

@nfcampos

This comment has been minimized.

Contributor

nfcampos commented May 29, 2016

It’s no different from choosing whether to provide shouldComponentUpdate() today—it’s exactly the same decision third party component have been doing for a long time.
Good point, I was under the impression that it was a different decision because functional children would be pure as well but I now see that it is not. 
 in case of React Router, you’d only need to make your App top-level handler a PureComponent, for the rest of the app to work.
that does indeed work.
In any case, there should be no goal of “making every component pure”. It’s just an optimization that would cover many cases, and generally benefit apps that use immutability. You shouldn’t be chasing that optimization with every component in your app—React reserves the right to not enable it in some cases anyway.
if you treat props as immutable in some places of your app, you probably need to do that everywhere or you'll end up incredibly confused as to which components assume immutability and which don't. in that case why not make every component in the app pure?

_____________________________

From: Dan Abramov notifications@github.com
Sent: Sunday, May 29, 2016 1:38 PM
Subject: Re: [facebook/react] Add React.PureComponent, inherit purity for functional components (#6914)
To: facebook/react react@noreply.github.com
Cc: Mention mention@noreply.github.com, Nuno Campos nuno.campos@me.com

for an app that consists entirely of functional components to opt in to pure functional components the root component needs to be a class component (which extends PureComponent), it can't be a functional component itself, right?

This is correct.

presumably every library that renders user-provided components — eg. react-router, react-redux, etc. — will need to provide two versions of whatever internal component renders the user provided components, one that extends PureComponent and one which extends Component so that library users can be offered the choice of having their functional components further down the tree be pure or not. I guess this is not really a question, more of a realisation that it looks like this will end up being an option in the api of a majority of libraries once this gets released.

In some cases, but not necessarily. This is not much different from today: libraries have to take a stance on where they are on the mutability spectrum. For example React Redux already has pure class containers, so it will switch to PureComponent. (But it already provides pure: false opt-out, so I guess we’ll have to switch there.)

As for other libraries, it’s no different then the situation today. If you don’t want to force your users to be immutable, you can just export a regular class. If the consumer wants optimizations, they can wrap the children into their pure container: // not optimized // my own, optimized!

// functional, optimized thanks to MyApp

So I think

It seems that you would have to expose one non-pure and one pure of every third-party component

is unnecessary overkill, and doesn’t need to happen. If you’re not sure your users are immutable, just provide Component. It’s no different from choosing whether to provide shouldComponentUpdate() today—it’s exactly the same decision third party component have been doing for a long time.

I expect that most libraries will provide just Components, and if you want to opt into the optimizations, you just wrap your components in a PureComponent.

The optimizations kick in if the closest class parent is a PureComponent so, e.g. in case of React Router, you’d only need to make your App top-level handler a PureComponent, for the rest of the app to work.

In any case, there should be no goal of “making every component pure”. It’s just an optimization that would cover many cases, and generally benefit apps that use immutability. You shouldn’t be chasing that optimization with every component in your app—React reserves the right to not enable it in some cases anyway.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@glenjamin

This comment has been minimized.

Contributor

glenjamin commented May 29, 2016

If this worked before, it only worked because some component above it caused it to re-render by setState or forceUpdate. Since this function wouldn’t have PureComponents above it, the existing pattern wouldn’t break, so I don’t see why this is breaking compatibility.

To clarify again: this change doesn’t make all functional components pure. You still have to opt in by making their closest class parent a PureComponent. At this point we know that all children would be effectively pure anyway, because if the parent didn’t rerender, they wouldn’t either. This is why it’s safe to make them as pure in this situation.

I don't think this is quite the same thing as considering the functional components pure.

Adding an automatic shouldComponentUpdate would stop them updating when the parent re-renders. Inlining them into the parent would be equivalent to having the component always update.

These two actions are only equivalent if the component is actually referentially transparent - which is a property that cannot be known from outside in JS.

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

if you treat props as immutable in some places of your app, you probably need to do that everywhere or you'll end up incredibly confused as to which components assume immutability and which don't. in that case why not make every component in the app pure?

Sure, but that wouldn’t work for people already using mutation (e.g. some implementations of Flux, some perf optimizations, making React work inside existing apps with Backbone, etc). So we want to keep that use case.

Adding an automatic shouldComponentUpdate would stop them updating when the parent re-renders. Inlining them into the parent would be equivalent to having the component always update.

I’m not sure I follow your point. There is no inlining in this PR (at least, not yet). I thought you were saying that adding this heuristic can potentially break functional components that use something like Date inside them. Can you show a bigger example demonstrating how they would break?

@glenjamin

This comment has been minimized.

Contributor

glenjamin commented May 29, 2016

I thought you were saying that adding this heuristic can potentially break functional components that use something like Date inside them. Can you show a bigger example demonstrating how they would break?

I'm only on my phone at the mo, can do something a bit more concrete (although possibly contrived) later.

When a function component renders is entirely controlled by its parent, but what it renders with could be anything.

I think that:
Automatic shouldComponentUpdate would be a semantic change
Inlining functional components wouldn't be.

Here's a slightly contrived example that should demonstrate:

function LastRendered() {
  return <p>This component last rendered at {new Date().toString()}</p>;
}
@jquense

This comment has been minimized.

Collaborator

jquense commented May 29, 2016

agree with @glenjamin here, having components act differently based on there parent seems a bit crazy, and that not the only thing determining pure essential of the component. I appreciate the attempts to optimize function components however this sort of heuristic feels really leaky.

as it is parent components have to sometimes be aware of their children types in order to attach refs, that leanings is less bad since refs are an escape hatch. With this we also have to be aware that changing a parent might cascade pure render checks down. thinking about libraries that wrap children to attach refs or refactoring a parent fn component to be statefull bc it's needed now.

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

With this we also have to be aware that changing a parent might cascade pure render checks down.

How would that be noticeable? If you made the parent pure you already effectively short-circuited it. What happens below is just an additional optimization that React can make now; it doesn’t affect your behavior. Can you show an example demonstrating why this could be a problem?

@satya164

This comment has been minimized.

satya164 commented May 29, 2016

@gaearon IMO the pureness of component should be up-to the component. A parent can not always know if a child component is pure or not.

For example, the following is always a pure component, regardless of its parents purity,

const Badge = ({ count }: { count: number }) => <span style={{ color: 'red' }}>{count}</span>;

The following is always impure,

const LastTime = () => <span>{Date.now()}</span>;

These are very bad examples, I know, but this demonstrates the fact that the child can be pure/impure regardless of its parent, and it should be up to the child to define its behaviour.

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

I think some confusion comes from us giving another meaning to “pure” when we say “component”.

React never allowed render function to be impure. That is, components already have to have pure render function per the documentation:

The render() function should be pure, meaning that it does not modify component state, it returns the same result each time it's invoked, and it does not read from or write to the DOM or otherwise interact with the browser (e.g., by using setTimeout). If you need to interact with the browser, perform your work in componentDidMount() or the other lifecycle methods instead. Keeping render() pure makes server rendering more practical and makes components easier to think about.

So functions that use something like Date().now are not officially allowed anyway, and won’t even work unless by some coincidence a parent component redraws often enough.

In the context of this discussion, “pure component” means that not only it is a pure function (which it already must be anyway), but it also doesn’t rely on any deep mutations in the props. In other words, it means that the props are immutable rather than the function is pure. And whether the props are immutable or not, depends on the caller, and not on the component itself.

Sorry about the confusion.

@jquense

This comment has been minimized.

Collaborator

jquense commented May 29, 2016

the examples above of non pure components with "pure" props all would/could change behavior depending on if the parent changes its component type. more so tho assuming that functional components that are the children of functional components are also eligible for pure render checks. you can easily create/break chains when refactoring components along the hierarchy no? That all feels like a bit of hard to track down magic that could case subtle bugs.

Not to mention the issues around context propagating through pure components

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

(Of course React allows impure render functions in the sense that you can call forceUpdate() to force-render them. This doesn’t go away. You can also setState({}) to force re-rendering. In this case, the implementation proposed in PR would also disable the optimization.)

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

you can easily create/break chains when refactoring components along the hierarchy no? That all feels like a bit of hard to track down magic that could case subtle bugs.

The idea is you shouldn’t have to think about it at all. This heuristic is intended to be an optimization React makes on its own; not something you need to be aware of. Because it doesn’t break the existing scenarios (does it?) it is effectively safe to add, so it’s a nice-to-have feature that you shouldn’t have to think about it.

Not to mention the issues around context propagating through pure components

It’s not any different than components with shouldComponentUpdate. Either implementing shouldComponentUpdate or inheriting PureComponent is an opt-in decision. Once you do so, you already break the context updates for all the children. So adding PureComponent and the optimizations related to it doesn’t make the situation any worse than it already is.

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

could case subtle bugs.

Could you please provide a specific example that demonstrates how it causes a bug? It would need to show that adding PureContainer with the functional component optimization is more dangerous than adding shouldComponentUpdate (without functional optimization) in the same place.

Otherwise this discussion goes into a very theoretical tangent 😄 .

@syranide

This comment has been minimized.

Contributor

syranide commented May 29, 2016

it’s exactly the same decision third party component have been doing for a long time.

@gaearon Not sure if that's a good argument considering it's an issue right now. :P But yeah, the rest of your argument seems sound. I guess in a sense you mark your "logic containers" as pure (because they deal with the data) and the children (being passed the data also inherits pureness) and it makes sense because they're mostly concerned with presentation only.

The optimizations kick in if the closest class parent is a PureComponent so, e.g. in case of React Router, you’d only need to make your App top-level handler a PureComponent, for the rest of the app to work.

Won't this break components that deal with mutable data internally? Even more so considering your example, ReactRouter must not mark itself as pure or my entire app must then be pure?

I may be missing something, but it seems like there should be three states; PURE, NON-PURE and INHERIT. So that you can go from being pure to being non-pure for instance, because you may be using some isolated component that may be internally non-pure and it must not inherit pureness. Or?

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

Won't this break components that deal with mutable data internally?

I have an answer for this:

Could you please provide a specific example that demonstrates how it causes a bug?

😄

We’re getting too abstract and might mean some different things; code will help.

Even more so considering your example, ReactRouter must not mark itself as pure or my entire app must then be pure?

Not entire app, just the descendants between React Router and next impure class. React Router marking itself as pure would have the same effect as React Router implementing a strict shouldComponentUpdate. To cause re-renders below, you’d have to use setState() or forceUpdate(). But these work with the new model as well!

@gaearon

This comment has been minimized.

Member

gaearon commented May 29, 2016

Functional components can’t setState() or forceUpdate(), right? So if functional component is inside a class component with strict shouldComponentUpdate() we know it won’t get re-rendered unless that parent also gets re-rendered. This is why we apply the heuristic here (and use PureComponent as a flag because we don’t know what’s in your shouldComponentUpdate()): functional component’s closest class parent is known to be pure.

If we introduce an impure class component between them, this will no longer be true. Maybe it calls setState({}) every second, and this causes functional component to update. This is why we don’t apply the heuristic here: functional component’s closest class parent is impure, so we don’t risk.

@jquense

This comment has been minimized.

Collaborator

jquense commented May 29, 2016

general bikeshedding aside. it strikes me that a change like this would benefit the most from benchmarking. Since this is all about performance anyway, a top criteria for acceptance probably should be "is the reasonably faster" for real world usages? Childless leaf nodes still run SCU? does that cost less than a reconciliation check? In the past I've only used SCU to gate updates on hot paths, so tips a good cost to benefit ratio. Is that still the case when many components, including simple ones, are doing SCU checks? (I don't know! but it seems like data would be helpful for this discussion.)

@koba04

This comment has been minimized.

Contributor

koba04 commented May 31, 2016

I wrote some tests for understanding clearly what PureComponent is.
koba04@e2eb5d6

#6914 (comment)
In this case, we need to add a Component wrapping MyFunctionalComponent for making it pure?

import React from 'react';
import Panel from 'react-bootstrap/lib/Panel';

class MyPureComponent extends React.PureComponent {
  /* ... */

  render() {
    return (
      <Panel defaultExpanded={false}>
        <PureContainer>   // makes MyFunctionalComponent pure
          <MyFunctionalComponent data={this.props.data} />
        </PureContainer>
      </Panel>
    );
  }
}
@taion

This comment has been minimized.

taion commented May 31, 2016

Doesn't work if there's more than one child.

Inherit purity for functional components
We've heard clearly that most React users intend for their functional components to be pure and to produce different output only if the component's props have changed (https://mobile.twitter.com/reactjs/status/736412808372314114). However, a significant fraction of users still rely on mutation in their apps; when used with mutation, comparing props on each render could lead to components not updating even when the data is changed.

Therefore, we're changing functional components to behave as pure when they're used inside a React.PureComponent but to rerender unconditionally when contained in a React.Component:

```js
class Post extends React.PureComponent {  // or React.Component
  render() {
    return (
      <div className="post">
        <PostHeader model={this.props.model} />
        <PostBody model={this.props.model} />
      </div>
    );
  }
}

function PostHeader(props) {
  // ...
}

function PostBody(props) {
  // ...
}
```

In this example, the functional components PostHeader and PostBody will be treated as pure because they're rendered by a pure parent component (Post). If our app used mutable models instead, Post should extend React.Component, which would cause PostHeader and PostBody to rerender whenever Post does, even if the model object is the same.

We anticipate that this behavior will work well in real-world apps: if you use immutable data, your class-based components can extend React.PureComponent and your functional components will be pure too; if you use mutable data, your class-based components will extend React.Component and your functional components will update accordingly.

In the future, we might adjust these heuristics to improve performance. For example, we might do runtime detection of components like

```js
function FancyButton(props) {
  return <Button style="fancy" text={props.text} />;
}
```

and optimize them to "inline" the child Button component and call it immediately, so that React doesn't need to store the props for Button nor allocate a backing instance for it -- causing less work to be performed and reducing GC pressure.
@ghost

This comment has been minimized.

ghost commented Jun 1, 2016

@spicyj updated the pull request.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jun 1, 2016

Updated with isParentPure instead of pureParent.

this._debugID,
'shouldComponentUpdate'
);
var pureSelf =

This comment has been minimized.

@gaearon

gaearon Jun 6, 2016

Member

Similarly, could call this isSelfPure for clarity.

@chicoxyzzy

This comment has been minimized.

Contributor

chicoxyzzy commented Jun 17, 2016

FYI PureComponent may be unnecessary not only if you use mutation but also if you are sure you get new value each time.

Let's say we have WebSocket connection and backend does all checks to be sure it sends only new data to client. Then (using Redux, Flux or any other mutable or immutable approach) you change store (or whatever) or immidiately pass props to component. In that case any new props should trigger rerender immidiately and any shallowEqual checks will be redundant. Same for cases when one uses Rx and combineLatest.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jun 29, 2016

I was looking forward to using this.
Is it still going to be merged, or some other approach being considered?

@gaearon

This comment has been minimized.

Member

gaearon commented Jun 30, 2016

Is it still going to be merged, or some other approach being considered?

Yes, this will be merged.

@taion

This comment has been minimized.

taion commented Jun 30, 2016

Are there any updates relating to #6914 (comment) and surrounding conversations?

Is the recommendation still that libraries offering container components should in general use Component instead of PureComponent for those container components?

@gaearon

This comment has been minimized.

Member

gaearon commented Jun 30, 2016

Is the recommendation still that libraries offering container components should in general use Component instead of PureComponent for those container components?

Yep, that’s the recommendation for now. We may adjust and extend the heuristic later—this shouldn’t stop us from adding PureComponent now.

@jimfb

This comment has been minimized.

Contributor

jimfb commented Jun 30, 2016

@gaearon We need to solve this heuristic situation. We should not merge something with a broken heuristic. The bugs that this would introduce are too difficult to debug because the reason for the seemingly random failure is not visible by inspection and difficult to reproduce (unless you already know what you're looking for). We don't want people banging their heads against the wall for hours, trying to understand what went wrong.

We can merge PureComponent without the heuristic (figure out a heuristic later if we don't want to block PureComponent), or with a smarter heuristic that passes #6914 (comment) (like owner-based), but not with the current heuristic.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jun 30, 2016

with a smarter heuristic that passes #6914 (comment) (like owner-based)

Do you mean the context test linked there? You basically can't rely on context updates propagating anyway today so I don't think that is a problem.

@jimfb

This comment has been minimized.

Contributor

jimfb commented Jun 30, 2016

@spicyj Err, no, sorry, I meant:

class PureComponent extends React.PureComponent{
  render(){ return <div>{this.props.children}</div>}
}

function MyFunctionalInputComponent(props) {
  return <input type="text" value={props.data.text} onChange={props.data.handler} />
}

class Impure extends React.Component {
  constructor() {
    super();
    var data = {};
    data.text = "Jim";
    data.handler = (event) => { data.text = event.target.value; this.forceUpdate(); };
    this.state = {data: data};
  }
  render() {
    return <PureComponent name="foo">
      <MyFunctionalInputComponent data={this.state.data} />
    </PureComponent>
  }
}

ReactDOM.render(
  <Impure />,
  document.getElementById('container')
);

When writing this code, everything looks right. When I write my pure component, it looks pure, so that's good. I write my stateless functional component, it looks stateless and functional, so that seems good. I write my Impure component, it is impure, and that's good. When I put them together, it breaks, and I have no idea why.

The problem is that a user would never expect that the data being passed to MyFunctionalInputComponent is actually captured as a deeply nested prop of PureComponent, and is thus illegal. It feels like I'm following all the rules, because it feels like I'm only passing pure data to my pure component. My functional input component is just a function, and I expect it to behave like a function. Everything looks very reasonable. For an even moderately complex component, you'd never spot the bug. Even after I explain it to people, most people have trouble realizing what went wrong. The story is absolutely terrible.

@taion

This comment has been minimized.

taion commented Jun 30, 2016

@jimfb

The interesting case with your <PureComponent> above is that making it pure accomplishes almost nothing – the shallow comparison is always going to return "unequal" because of the children prop. If there were the concept of a component that passed through purity, without any particular SCU semantics, it would be just fine for that example.

Would it be reasonable to not apply the heuristic at all here for functional components? If I want to take advantage of the logic here, I'm going to have to change all of my components to extend PureComponent anyway. Is it that big a deal if I also have to do e.g. MyFunctionalComponent.pure = true? I already have to bind a bunch of static properties to those components like propTypes anyway.

Alternatively, a really conservative heuristic that just impure-ifies everything any non-pure component seems like it would avoid potential problems. As a library maintainer I'd just cut all of my library components over to PureComponent, so in practice I think things would be okay.

@martin-svk

This comment has been minimized.

martin-svk commented Jul 5, 2016

This one is tricky.

Firstly, it seemed like a no big deal improvement. Just a shorter way to implement a SCU method with shallowCompare(this, newProps, newState). Many arguments pointing to a concept of owner should control it's own purity/rerending are invalid because we already have SCU method deciding if the child components (those not setting their own state) will be rerendered or not. Also, many examples were totally wrong, having impure render methods.

But the problem is, this PR takes this concept further and tries to apply some additional heuristics which can have unexpected causes. Like #6914 (comment) and #6914 (comment).

What I suggest is to just make all the functionall components behave like they had SCU implemented using shallowCompare on props, assuming users use immutable data as props. There will be a clear warning about this in docs. And people which want to do mutation can stick with classic Component's.

Additionally if PureComponent only did implement the SCU as I mentioned, I would not be against it.
Actually, that's what other people were already doing to save some typing, and what PureRenderMixin was all about, right?

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jul 5, 2016

Gonna do the first half of this in #7195.

@sebmarkbage

Changes requested above.

@sophiebits sophiebits closed this Mar 1, 2017

@markerikson markerikson referenced this pull request Mar 4, 2017

Merged

FSC post #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment