Remove JSXElement from JSXAttributeValue production?

[RyanCavanaugh]

We just got a bug on TypeScript from someone who was working on some ESLint stuff and found that the TypeScript parser didn't support JSX Elements as Attribute Values. This was news to us because we didn't realize the spec had been changed, and because we had gotten no bug reports from people trying to do this for real.

Babel doesn't support this correctly either:

var x = <foo bar=<qua /> />;

Property value of JSXAttribute expected node to be of a type ["JSXElement","StringLiteral","JSXExpressionContainer"] but instead got "CallExpression"

It'd be trivial for us to implement parsing this, but I'd like to raise this as a potential simplification of the JSX spec given the number of extant consumers.

With no support among the two major JSX transpilers and no user complaints to that end, perhaps this is safe to remove for the sake of simplicity? Adding { } around the attribute value doesn't seem like too much of a burden.

See also #10 / #15

[RReverser]

Babel doesn't support this correctly either:

Hmm, it definitely did earlier, and from the error:

repl: Property value of JSXAttribute expected node to be of a type ["JSXElement","StringLiteral","JSXExpressionContainer"] but instead got "CallExpression"

It looks like it's just a regression bug. /cc @kittens

[NekR]

And nobody spotted it. At least, it could be marked deprecated or something
and TS will just ignore then.
On Mar 8, 2016 1:10 PM, "Ingvar Stepanyan" [email protected] wrote:

Babel doesn't support this correctly either:

Hmm, it definitely did earlier, and from the error:

repl: Property value of JSXAttribute expected node to be of a type
["JSXElement","StringLiteral","JSXExpressionContainer"] but instead got
"CallExpression"

It looks like it's just a regression bug. /cc @kittens
https://github.com/kittens

—
Reply to this email directly or view it on GitHub
#53 (comment).

[RReverser]

we didn't realize the spec had been changed

In fact, this feature here from the very beginning.

And nobody spotted it.

Well, from

because we had gotten no bug reports from people trying to do this for real

I would say people use it.

As for Babel, many just didn't switch to 6.x yet so could easily not notice it yet.

[JamesHenry]

(Original bug reporter here)

@NekR there is nowhere near enough evidence for you to make that judgement. People who use it and babel could still be using a version which supports it, for example.

[JamesHenry]

Thanks for your input on this everyone.

I have had chance to think about this a bit more and IMHO this feels like the wrong motivation for a change to the spec.

We do not have a way to reliably say what the usage of the feature is like, and the actual concern is simply that two (very popular) implementations of the JSX spec (TypeScript compiler and Babel) have seemingly not implemented it. (I can't comment on the regression in Babel too much, but I am more than happy to submit an issue to the babel repo if you think that would be useful, @kittens)

There are other things which interpret JSX that I know have implemented it e.g. espree and acorn-JSX, but I am sure there are also others.

Happy to discuss it more thoroughly, but right now it seems like a good way to resolve this would be that I work with @RyanCavanaugh to ensure that the TypeScript compiler can support this, and follow up with Babel based on @kittens feedback.

Let me know what you guys think.

[RyanCavanaugh]

In fact, this feature here from the very beginning.

@RReverser I'm very confused by this statement; what do you mean by it? It was very clearly added to the spec with PR #15 which happened after the JSX spec was first public.

[RyanCavanaugh]

Seems at least one person cares about it, which is good enough for me.

[RReverser]

It was very clearly added to the spec with PR #15 which happened after the JSX spec was first public.

@ RyanCavanaugh Sorry for the confusion - I mean that just like other features, it was added to the spec post-factum as the feature itself already existed in all the available implementations at that point (esprima-fb and acorn-jsx) and simply was missed when writing the spec.

[RyanCavanaugh]

The circle of life 😄

1. In the old implementation, but not in the spec
2. In the spec, but not in the new implementation
3. In the spec and in the new implementation 🎉

[NekR]

@JamesHenry

It looks like it's just a regression bug.
And nobody spotted it.

If you have any evidence that Babel users spotted it or had problems with it, then just leave them here and I will stop judging. Is it fine for you?

[JamesHenry]

This thread has been resolved, thanks for your input @NekR.

[NekR]

@JamesHenry I know, but I may write here as many as I want, until owners came here and say my comments are inappropriate, which were not because I just left my thoughts and this is why is all are happening in open -- to let people left their thoughts here, in normal way.

One thing which I cannot understand is you coming here and starting talking to me like you are the owner of JSX and you do not like my comments here. if you think someone is wrong, just say that you think other way. Nobody needs your hypocrisy like this: thanks for your input.

[JamesHenry]

Let's try and keep things positive and on topic. I am happy to chat further if you wish to reach out privately, but I think we can draw a line under this now. Sorry for any confusion, there was no animosity intended from my side.

[sophiebits]

(We added this to the parser originally in facebookarchive/esprima#9 but never made the transformer (jstransform at the time) support it.)

[RReverser]

@spicyj That's weird, I remember actively using it way before Babel.

[sophiebits]

@RReverser Hmm? I didn't mention Babel.

[RReverser]

@spicyj I know, that's what I'm saying - IIRC, I remember using this feature with the original React's transpiler.

[RyanCavanaugh]

I'd just like to raise this up again as it's been over a year and Babel still doesn't support it and TypeScript still doesn't support it, without major complaint as far as I can tell. Seems like the only people who are using this are people who were using JSX in its infancy and no one else knows it's even supposed to be possible. Thoughts?

[loganfsmyth]

We just fixed this for Babel 7 FYI: babel/babel#6006 Babel's parser has supported it for a long time, but I guess the transform broke at some point.

[nojvek]

I like the simplicity of {} implies expression i.e x={y} and <tab>{expr}</tab> syntax. I'd vouch for simplifying JSX and removing this special case and typescript not implementing the x=<tag/> hack.

Personally I don't even use the x="abc" syntax since it is a special case, x=1 or x=true don't work. I instead always use x={"abc"}. It's simpler to always get around.

Same with children <div>{"message"}</div>. That way it's very explicit where the whitespace is.

[Huxpro]

Hmm... What's the 2022 (OH MY) status of this?

It seems like Babel support this since babel/babel#6006 and TS still doesn't? And it seems like Flow doesn't support this as well (facebook/flow#7546)?

If both TS and Flow doesn't support this I think we should just make an normative change to remove this. Thoughts?

[wooorm]

I also didn’t implement it in MDX, as I never see it used in the wild and there’s a good alternative (add braces).
I’m 👍 on removing it. (And from what I see here, and not landing it in TS in 5 years, it seems to me that most folks here are too)

[RyanCavanaugh]

microsoft/TypeScript#47994 was recently posted and looks pretty ready to merge IMO. There aren't any blockers from the TS side to supporting this syntax, it was just a matter of no one having bothered to implement it. Original issue is microsoft/TypeScript#7410 which sat "help wanted" for ~5 years 🙂

[RyanCavanaugh]

@Huxpro the PR to support it on TS is ready to merge and I'm inclined to do so to match the spec and Babel unless you say otherwise. It's not much of a burden for parsers IMO since the legal grammar productions at this point are very constrained.

[wooorm]

One more case that I forgot for why I didn’t implement it in MDX: <a b=<c>*a*</c> />. Is that an <em>? Is there a <p> too? As the braced version (<a b={<c>*a*</c>} />) is clear (JavaScript, so markdown doesn’t work), and given that nobody has asked for this feature, I’m not going to implementing it. That’s very specific to MDX though.

I can understand TS merging that PR and this issue being closed. I’d personally 👍 this 5 year old request to simplify the spec, and remove this functionality.

[RyanCavanaugh]

I also find it personally distasteful, but just have no technical objections to it 😅