Add optimization to ReactDOMComponent

[petehunt]

@sebmarkbage looks like we missed this one

[syranide]

@petehunt Is that actually valid "today"? Are you allowed to actually keep the reference to a "virtual component" and reuse it?

[petehunt]

Think so... figuring it out now :)

[syranide]

@petehunt IIRC I think it works, but you'll run into issues if the "virtual component" unmounts and then mounts again. But if that's true, I'd say that's a bug rather than a blocker to this optimization.

Hmm, also, I guess this optimization theoretically assumes that the user has immutable props, since the component reference could remain, but an object inside one of its props could have changed.

[sebmarkbage]

I think this diverged upstream. I'll let @zpao figure this one out.

[sebmarkbage]

@syranide if you pass a component descriptor as props, then the thing you pass it to may rerenders without you rerendering.

I'm not sure what you mean by the unmount scenario but I think this won't be a problem once we stop mounting the same instance and have true descriptors.

[syranide]

@sebmarkbage Yeah, but what I mean, AFAIK React prefers immutable props/data but is implemented to work perfectly well with mutable data (i.e, it doesn't check prevProps === nextProps for that reason I believe). nextComponent === this goes against that right? If using mutable data then nextComponent === this can be true while the data has changed.

As for the unmount scenario, I remember someone having an issue with reusing component descriptors, it kind of actually worked correctly, but when the same component descriptor got remounted the life cycle methods would behave weirdly. I.e, it seemed to me like this was an optimization for a case that doesn't actually work right now (also, no it shouldn't be a problem when they are true descriptors).

[sebmarkbage]

Remounting the same descriptor again only breaks if you switch back and forth between two of them and we're fixing that with the 0.11 release anyway.

The typical pattern for mutable objects in state is that you rerender the component which expects updates. If you rerender the component that pass props, then you should be recreating the descriptor.

var Outer = React.createClass({

  render: function() {
    var child = <div style={{ color: 'black' }} />;
    return <Inner child={child} />;
  }

});

var Inner = React.createClass({

  getInitialState: function() {
    return { clicked: false };
  },

  handeClick: function() {
    this.setState({ clicked: true });
  },

  render: function() {
    return <div onClick={this.handleClick}>{this.props.child}</div>;
  }

});

You could modify the props of the outer component for this to break:

  handleClick: function() {
    this.props.child.style.color = 'white';
    this.setState({ clicked: true });
  }

This is already a terrible pattern that is broken for many other cases. If you mutate objects, it should at least be your own. transferPropsTo already explicitly isn't allowed on components owned by someone else for this reason.

Another way to break it would be by creating the instance outside the render flow.

var child = <div style={{ color: 'white '}} />;

var Outer = React.createClass({

  handleSomething: function() {
    child.props.style.color = 'black';
  },

  render: function() {
    return <Inner child={child} />;
  }

});

The original proposal was to do this optimization using the owner level as a reference. That would solve the last case where the child doesn't have an owner and therefore isn't recreated.

Enforcing strict recreation also allow us to reason better about pooling since we know for sure that old instances are no longer needed.

So in short. I think that we can still allow mutation of state objects in most cases but you need to recreate component descriptors in the thing that is rerendering.

[sebmarkbage]

oh this is not in ReactCompositeComponent. Then this PR makes sense as is.