wix-incubator / react-templates

Light weight templates for react
https://wix.github.io/react-templates
MIT License
2.82k stars 206 forks source link

Anyway to concisely pass in all props to a component? #136

Open steezeburger opened 8 years ago

steezeburger commented 8 years ago

W/o react-templates <Cart {...this.props} />

w/ react-templates <Cart rt-props="{ cart: this.props.cart, gotoCheckout: this.props.gotoCheckout, etc... }" />

nippur72 commented 8 years ago

By looking at how JSX spread is turned into JS, you can write:

<Cart rt-props="React.__spread({},this.props)">Hello</Cart>

it's not very elegant, but better than listing properties one by one.

Perhaps this could evolve into a special rt- attribute like rt-spread-props or similar.

yotambarzilay commented 8 years ago

I think I'm missing something - wouldn't the following rt be enough to pass all props from the parent to its child?

<Cart rt-props="this.props" />
nippur72 commented 8 years ago

@yotambarzilay yes but that does not create another object instance. If you mutate props in the parent, you get the change also in the child (of course I'm aware that's a discouraged practice).

For completeness, React's documentation mentions also Object.assign() and _.extend as alternative methods.

steezeburger commented 8 years ago

I tried that @yotambarzilay but it never worked. I got syntax errors and it wouldn't compile. I ended up using lodash's _.assign() and it worked ( Object.assign() worked as well ).

nippur72 commented 8 years ago

@steezeburger here's a jsfiddle with this.props that compiles without syntax errors, in case you want to check. Anyway I'd prefer one of the above mentioned alternatives, because they all create a new instance.

steezeburger commented 8 years ago

Interesting. I'm not sure what I was doing wrong. Probably something silly.

Should I close this or leave it open for discussion of a new syntax?

nippur72 commented 8 years ago

Leave it open, I'll try to submit a PR for rt-spread-props, it should be easy.

yotambarzilay commented 8 years ago

I don't think a new attribute, which is basically an alias for a clone/assign function, is the best way to go. What do you think of supporting the es6 spread operator? E.g. rt-props="...this.props" On Jun 3, 2016 6:13 PM, "nino-porcino" notifications@github.com wrote:

Leave it open, I'll try to submit a PR for rt-spread-props, it should be easy.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wix/react-templates/issues/136#issuecomment-223606596, or mute the thread https://github.com/notifications/unsubscribe/AGrDDFlQZYOTjeuXzQVb6DdAhfwHvIRgks5qIESBgaJpZM4In6fm .

yotambarzilay commented 8 years ago

Or rather rt-props="{...this.props}" On Jun 3, 2016 7:12 PM, "Yotam Barzilay" yotamb@wix.com wrote:

I don't think a new attribute, which is basically an alias for a clone/assign function, is the best way to go. What do you think of supporting the es6 spread operator? E.g. rt-props="...this.props" On Jun 3, 2016 6:13 PM, "nino-porcino" notifications@github.com wrote:

Leave it open, I'll try to submit a PR for rt-spread-props, it should be easy.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wix/react-templates/issues/136#issuecomment-223606596, or mute the thread https://github.com/notifications/unsubscribe/AGrDDFlQZYOTjeuXzQVb6DdAhfwHvIRgks5qIESBgaJpZM4In6fm .

steezeburger commented 8 years ago

Personally, I would rather support the spread operator than add new syntax/api.

nippur72 commented 8 years ago

I am for supporting the spread operator too, but some ES6->ES5 transformation is needed.

I quickly checked esprima/escodegen and the ... doesn't seem to be supported (which is strange).

React-templates internally uses esprima/escodegen to check and format JS code, so my idea would be to intercept the spread operator in the AST tree and turn it into the corresponding React.__spread call.

Any better idea?

nippur72 commented 8 years ago

EDIT: the reason why it's not supported in esprima/escodegen is because it's an ES7 feature (proposed) not an ES6 one. So hacking the AST tree is no longer an option.

vinhhungle commented 7 years ago

+1 for rt-spread-props

Currently, I use Object.assign because there's Warning: React.__spread is deprecated and should not be used.

<Card rt-props="Object.assign({item: item, key: itemIndex}, props)" />