stampit-org / react-stamp

Composables for React.
370 stars 10 forks source link

Remove special React composition logic? #3

Closed troutowicz closed 8 years ago

troutowicz commented 8 years ago

A recent issue over at the React repo (https://github.com/facebook/react/issues/5047) got me thinking.

When I wrote this library originally (react-stampit), I wanted to enforce composition logic similar to React.createClass. More about this is described in the docs here. But after reading and thinking about the above React issue, maybe this is the wrong thing to do. What if react-stamp simply stuck to the composition logic layed out in the stamp spec while providing the React-like API? No method wrapping, no duplicate key checks, just assign/merge all properties with last-in priority.

I'm having trouble coming up with examples favoring the current composition logic.

cc @ericelliott

ericelliott commented 8 years ago

What about component lifecycle hooks? Don't you think all the hooks should run (except .render())? How were you planning to handle that?

troutowicz commented 8 years ago

React lifecycle methods would just use the same method composition as any other method. (override with last-in priority). So if a lifecycle method exists in a mixin and the base component, the lifecycle method in the base component would get overridden. I can see this might not be the behavior we would want.

Aside from the lifecycle methods, thoughts on removing the React logic for component state and static properties? Is there any particular reason we would care about duplicate keys? I think being less opinionated there might make more sense. (code).

ericelliott commented 8 years ago

I can see this might not be the behavior we would want.

Probably not at all what you'd want for the lifecycle methods. Wouldn't it break a lot of components?

thoughts on removing the React logic for component state and static properties?

I'd probably want to treat state and static properties like deepProperties. What do you think?

troutowicz commented 8 years ago

Wouldn't it break a lot of components?

It would be new behavior, so yeah probably. Let's keep the React lifecycle method wrapping as is.

I'd probably want to treat state and static properties like deepProperties. What do you think?

I was thinking deepStaticProperties for the statics, and deepProperties for state.

Edit: For anyone else reading this, we are referring to the descriptor properties in the stamp specification.

ericelliott commented 8 years ago

I was thinking deepStaticProperties for the statics, and deepProperties for state.

Yep. That's what I meant. =)