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

Allow Expression inside expression container #17

Closed RReverser closed 2 years ago

RReverser commented 9 years ago

Subject to discuss as per facebook/react#2143.

sebmarkbage commented 9 years ago

One way to think about it, is as an object initializer, function call or an array initializer and those don't allow Expression. Just AssignmentExpression. There's actually very few places where Expression is allowed.

The only reason we can do it right now is because of the braces around it. If we allow the curlies to become optional for certain subset expressions, it might make sense to disallow it in general.

I'm not sure. I tend to agree that, if we can easily allow it, we should.

RReverser commented 9 years ago

One way to think about it, is as an object initializer, function call or an array initializer and those don't allow Expression. Just AssignmentExpression. There's actually very few places where Expression is allowed.

In the case under "object initializer" and "array initializer" you mean literals, they can actually contain SequenceExpression as child as per Mozilla AST specs (as well as CallExpression can). The only reason they can't have SequenceExpression without parenthesis is because they're all already comma-separated lists - of properties, of array elements or of arguments. It's not our case though.

In our case, we have single expression inside braces that can be unambigously parsed as Expression (either SequenceExpression or AssignmentExpression) and even better - esprima-fb and acorn-jsx both already parse it correctly.

It all becomes easier if we start treating (just in mind, not in code) braces in JSXExpressionContainer as special case of parenthesis - and then it becomes obvious how to handle their contents and why SequenceExpression actually works inside.

Also, as I already said, this is already supported and correctly handled by existing parsers, so in the case we allow it, the only change needed will be the one implemented in PR to React's jstransform rule that adds parenthesis around sequence expression for correct behavior inside another comma-separated list (object/array/call expression).

syranide commented 9 years ago

I'll reiterate my point from the earlier discussion. I don't see who would ever benefit from {a,b} == {(a,b)}. It's unintuitive and can be a source of confusion as anything before the final , is counter-intuitively discarded from the output. The vast majority don't even know JS supports (a,b), it's a tragic language feature that I'm sure has no legitimate use-case other than for its current heavy use for minified code.

I'm not saying we should go the way of {a,b} == {a}{b} but if we did it could be considered a circumstantially useful feature. return <Comp>{a, b, c}</Comp> would then desugar to return Comp(null, a, b, c) (or similar) which in my opinion is rather reasonable and could actually have some legitimate use-cases.

I don't see how {a,b} implies invisible parenthesis any more than it implies appending multiple expressions to the result. If you really absolutely want {(a,b)} then you should explicitly put the parenthesis, it's beneficial to everyone who reads the code.

I'm totally fine with throwing errors for {a,b}, but implying {(a,b)} makes no sense to me other than "because we can", throwing less compile-time errors is not inherently beneficial.

NekR commented 9 years ago

I think @syranide right in this situation, more over, that means that existing parsers has bug with it. I do not think it's good to be in situation like HTML living standard when spec should follow implementations, since JSX is young and {a, b} is very rare case and also, it's already prudeces error. So in my mind it should either throw more informative error (not side effect error), or produce something not {(a, b)}.

I'm not saying we should go the way of {a,b} == {a}{b} but if we did it could be considered a circumstantially useful feature. return {a, b, c} would then desugar to return Comp(null, a, b, c) (or similar) which in my opinion is rather reasonable and could actually have some legitimate use-cases.

Second case is more weird for me. How that case will work with non React.js JSX targets? Each should choose their own code to output for that case... I do not think it's good. Desugaring into {a}{b} is more correct for me, if that syntax really needed.

ghost commented 9 years ago

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

Huxpro commented 2 years ago

Personally I agree with @sebmarkbage and @syranide that allowing Expression is unwanted. Closed but feel free to reopen though.