search

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

Remove `JSXElement` from `JSXAttributeValue` production? #53

Open RyanCavanaugh opened 8 years ago

RyanCavanaugh commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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" notifications@github.com 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 https://github.com/facebook/jsx/issues/53#issuecomment-193731674.

RReverser commented 8 years ago

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 commented 8 years ago

(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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

RReverser commented 8 years ago

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 commented 8 years ago

The circle of life :smile:

  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 :tada:
NekR commented 8 years ago

@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 commented 8 years ago

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

NekR commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

RReverser commented 8 years ago

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

sophiebits commented 8 years ago

@RReverser Hmm? I didn't mention Babel.

RReverser commented 8 years ago

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

RyanCavanaugh commented 7 years ago

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 commented 7 years ago

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

nojvek commented 4 years ago

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 commented 2 years ago

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

It seems like Babel support this since https://github.com/babel/babel/pull/6006 and TS still doesn't? And it seems like Flow doesn't support this as well (https://github.com/facebook/flow/issues/7546)?

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

wooorm commented 2 years ago

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 :+1: 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 commented 2 years ago

https://github.com/microsoft/TypeScript/pull/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 https://github.com/microsoft/TypeScript/issues/7410 which sat "help wanted" for ~5 years 🙂

RyanCavanaugh commented 2 years ago

@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 commented 2 years ago

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 :+1: this 5 year old request to simplify the spec, and remove this functionality.

RyanCavanaugh commented 2 years ago

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