Incorrect handling of SequenceExpression inside XJSExpressionContainer

[RReverser]

Consider following examples:

React.renderComponent(<HelloMessage name={doSmth(), "Ingvar"} />, mountNode);

// Translates to:

React.renderComponent(HelloMessage({name: doSmth(), /* Syntax Error */ "Ingvar"}), mountNode);

or

React.renderComponent(<HelloMessage>{doSmth(), "Ingvar"}</HelloMessage>, mountNode);

// Translates to:

React.renderComponent(HelloMessage(null, /* Wrong output */ doSmth(), "Ingvar"), mountNode);

Sequence expressions are not used that often, but still they are so I believe both possible errors should be fixed.

Possible fix would be to treat XJSExpressionContainer as parenthesized expression (simply add "(" and ")" around inner expression in output).

[syranide]

What if we considered 2 a feature instead? It'd be rather close to facebook/jsx#8 and could turn out to be quite useful.

Sequence expressions shouldn't make sense inside {} as you're not allowed to have side-effects in render, sequence expressions are only useful for side-effects.

[RReverser]

I'm not sure it's correct to change meaning and way the JS expressions are executed based purely on our own views on what and where would be useful. If someone wanted to emit concatenated content, one would use + and not ,. Also, handling cases differently based on context would bring additional unnecessary checks in transpiler's logic, so I think it's better to just add parenthesizes for any XJSExpressionContainer with SequenceExpression inside.

[syranide]

@RReverser Not saying I disagree, but a + b is obviously not equal to a, b, {a}{b} is however. Users could also always access SequenceExpression with parenthesis {(a, b)} which I would arguably prefer as good practice. It's not immediately obvious that {a, b} is a SequenceExpression (EDIT, my intuition is that {a, b} === {a}{b}).

[RReverser]

@syranide Not saying I disagree, but a + b is obviously not equal to a, b,

They are not in JS, but with your assumption we're making those operators behave equally (both concatenate output). And that doesn't feel right, as we're changing the basic meaning of operators.

Users could also always access SequenceExpression with parenthesis {(a, b)} which I would arguably prefer as good practice.

Probably, but good practice is always still just a good practice and not a reason to change language semantics and operator meanings. Everyone who wants, can still use parenthesis inside braces, but if we say in spec that XJSExpressionContainer is {AssignmentExpression}, then we should strictly follow the spec and parse contents inside as AssignmentExpression (and SequenceExpression is one of it's possible subclasses).

[syranide]

They are not in JS, but with your assumption we're making those operators behave equally (both concatenate output). And that doesn't feel right, as we're changing the basic meaning of operators.

Hmm? No they're not, {'a' + 'b'} and {'a'}{'b'} does not have the same meaning or even necessarily the same result.

I don't agree that we're changing basic meaning of operators, abc{...}def is something entirely JSX-specific and the meaning of what's inside is ours to define, just like functions. f(a, b) is not a SequenceExpression and I don't see why {a, b} can't work the same way (not that it has to).

I'm not going to stand in the way, but to me {a, b} == {(a, b)} is useless and rather unintuitive, {a, b} == {a}{b} is somewhat intuitive and quite possibly useful (but perhaps not preferable). In my opinion even {a, b} == ERR might be preferable to {a, b} == {(a, b)}, it takes the guesswork out of the equation.

[RReverser]

{a, b} == ERR - hmm... maybe. Anyway, created a simple straightforward solution with parenthesis for now until further discussions on whether we should introduce error or not.

[sebmarkbage]

then we should strictly follow the spec and parse contents inside as AssignmentExpression (and SequenceExpression is one of it's possible subclasses).

I'm pretty sure that a sequence (known simply as Expression in the ES spec) is not included in AssignmentExpression. In fact, Expression is exactly just a sequence of AssignmentExpressions. I intentionally chose AssignmentExpression instead of Expression since I knew this wasn't supported.

If we want to support this, let's open up a task on the JSX spec and have the discussion there so that other interested parties can follow along in the rationale.

Leaving this bug open to chose either solution but, until we decide to fix the spec, the correct fix is to throw to a syntax error.

[RReverser]

I'm pretty sure that a sequence (known simply as Expression in the ES spec) is not included in AssignmentExpression.

You're right, my bad, sorry.

If we want to support this, let's open up a task on the JSX spec and have the discussion there so that other interested parties can follow along in the rationale.

Makes sense, created an issue for tracking: facebook/jsx#17.

Personally I prefer to allow any Expression inside braces and not throwing error as 1) we can do that easily and 2) personally for me throwing error where we can parse expression easily doesn't feel right (I just prefer less error-prone way).

Would be interesting to hear thoughts of other parties though.

[gaearon]

Closing as non-actionable in this repo.

[RReverser]

@gaearon Ha, thanks for bringing up some old memories!