facebook / jsx

The JSX specification is a XML-like syntax extension to ECMAScript.
http://facebook.github.io/jsx/
1.96k stars 133 forks source link

Support component class expressions #47

Open npryce opened 8 years ago

npryce commented 8 years ago

I would like JSX to support higher order components (e.g. functions that take and return components).

For example, given a component UserPortrait and a higher-order component ImageDropTarget (that wraps drag-and-drop image uploading around a component) I would like to be able to combine the two inline in the JSX for my UI, like so:

<ImageDropTarget(UserPortrait) src="blah-blah" onDrop={blah blah}/>

At the moment, one has to define temporary variables to use higher order components from within JSX. E.g.

var ImageDropTargetUserPortrait = ImageDropTarget(UserPortrait);
...
<ImageDropTargetUserPortrait  src="blah-blah" onDrop={blah blah}/>
RReverser commented 8 years ago

I believe this is confusing and unneeded syntax complication. Why can't you just nest components into each others in code?

NekR commented 8 years ago

Btw, JSX does not limits Component's variable to be of particular type. It's rather React and react-jsx transpilation behavior, so this issue should be discussed in ReactJS repo.

npryce commented 8 years ago

@RReverser nesting components (e.g. ) is different from composing new component classes (aka higher-order components).

The issue affects more than just higher-order components. Component classes are first-class values, but JSX does not allow one to use an expression for component type, so you have to introduce temporary variables that would be unnecessary in plain JavaScript and clutter up the code.

Could the existing {...} syntax for inserting JavaScript expression into JSX be used in the component name too?

<{ImageDropTarget(UserPortrait)} ...>
    ...
</{ImageDropTarget(UserPortrait)}>
RReverser commented 8 years ago

If you want to compose new class, then it will be most likely reused and you should really just store it in a variable. If you just want to use each specific combination once or two, then nesting is really the best option.

NekR commented 8 years ago

@npryce

<{ImageDropTarget(UserPortrait)} ...>
    ...
</{ImageDropTarget(UserPortrait)}>```

This does not looks better than this:

jsx`
<${ImageDropTarget(UserPortrait)} ...>
    ...
</${ImageDropTarget(UserPortrait)}>
`

This is why JSX captures variables. What is a problem with variables? In your code you instantiate ImageDropTarget twice, which is obviously worse than saving instance in variable (read: instance === composed class).

RReverser commented 8 years ago

@NekR

In your code you instantiate ImageDropTarget twice

I believe @npryce wants React to merge them automagically and instantiate once (which is too magical IMO).

syranide commented 8 years ago

@RReverser Could a dynamic type be justified because React reserves all lower-case names? This way you could have <(chosenClass) />, it could also aid readability with it highlighting it being a "dynamic" variable in a sense rather than an imported class. I don't really care and I'm not so sure this should extend all the way to full-blown expressions, just wanted to surface the idea.

RReverser commented 8 years ago

@syranide It just feels like this addition opens a wormhole that might lead to a worse code where we would see e.g.

return <{require('./myComponent').HigherComponent(YetAnotherHigherComponent(MyComponent))}>
  My Content
</{require('./myComponent').HigherComponent(YetAnotherHigherComponent(MyComponent))}>;

I don't really see why using local variable is so unfavorable if you want to combine and create new class.

As for lower-case names - probably true, although again, not so hard to use UpperCase variables for components.