Separate React Composite and Class

[sebmarkbage]

ReactClass is now composed by ReactCompositeComponent rather than
inherit from it.

The state transition functions currently use ReactInstanceMap to map an
instance back to an internal representation.

I updated some tests to use public APIs. Other unit tests still reach into
internals but now we can find them using ReactInstanceMap.

I will do more cleanup in follow ups. The purpose of this diff is to
preserve semantics and most of the existing code.

This effectively enables support for ES6 classes. All you would need to
expose is ReactClassMixin.

[sebmarkbage]

@spicyj @syranide Any thoughts?

[syranide]

@sebmarkbage I don't really feel all that familiar with these specifics so it's a bit beyond me, but I think everything you said makes sense. You've probably talked this through in a meeting, but could it make more sense to expose it as ReactComponentMixin, that's what we refer to instances as (I think?), ReactClassMixin sounds kind of generic, it's a "class mixin for React".

EDIT: PS. What happens to auto-binding? ES6 classes don't come with that by default AFAIK.

[sebmarkbage]

The idea is to expose whatever is on ReactClassMixin as a base class. Such as class MyComponent extends React.Component since that's more idiomatic in ES6 world.

Yea, auto-binding wouldn't work. You have to explicitly bind, or use ES.next property initializers to do the auto-binding: class MyComponent { myMethod = () => { } } That works in TypeScript. This is one place where we wouldn't have feature parity. We could build it into the base-class but it's kind of a magical feature. Not sure where we'll land but currently I'm leaning towards making it explicit somehow.

[syranide]

We could build it into the base-class but it's kind of a magical feature.

That sounds like a bad idea, your work on moving away from all magic stuff is great. I could see a benefit in exposing an auto-bind enhanced base class (preferably as addon) for those who are still OK with it until ES7+ provides a proper solution. Especially as it is preferable performance-wise to use auto-binding over manually binding (but perhaps the actual benefit is minimal?).

[sebmarkbage]

The actual benefit is minimal. It helps with referential equality checks though. You can have a shouldComponentUpdate = props => this.props.onClick !== props.onClick

[syranide]

@sebmarkbage The equality check is already somewhat compromised for those of us not religiously componentizing and sometimes bind index/id to callbacks. Perhaps one might even shed some composite component house keeping overhead (at creation) from dropping the auto-binding? That would be a win, but I imagine it would be "minimal" as well... anyway, I'm personally all for as little magic as possible "by default/in core".

[sophiebits]

+1 for less magic. Using ES7 property initializers with arrow functions is very appealing to me, though I understand the reluctance to introduce more syntax or a dependence on unstandardized features.

Haven't had a chance to read through the diff yet.

[zpao]

come on

[sebmarkbage]

I addressed the comments so... stamp?

[zpao]

I apparently add labels from mobile so 👍