Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update spec for putting JSXFragment inside attributes #94

Merged
merged 1 commit into from
Oct 12, 2017

Conversation

clemmy
Copy link
Contributor

@clemmy clemmy commented Oct 12, 2017

I think this is desired behavior and something that was overlooked in #93 - the idea is to allow fragments to be a part of JSX attributes. For example,<MyComponent attrib={<></>} />.

@sebmarkbage sebmarkbage merged commit 89f2cc0 into facebook:master Oct 12, 2017
@sebmarkbage
Copy link
Contributor

sebmarkbage commented Oct 12, 2017

The PR is correct but your example is wrong. That was already allowed. This PR makes it possible to have this: <MyComponent attrib=<></> />

@clemmy
Copy link
Contributor Author

clemmy commented Oct 12, 2017

@sebmarkbage

Ah, interesting. I wasn't aware of that - I guess Babel doesn't have that implemented yet. For example, if I try to transform <a x=<x></x>></a>, I get Property value of JSXAttribute expected node to be of a type ["JSXElement","StringLiteral","JSXExpressionContainer"] but instead got "CallExpression".

(https://babeljs.io/repl/#?babili=false&browsers=&build=&builtIns=false&code_lz=DwQwBAHgvMEHzAPTwYkcg&debug=false&circleciRepo=&evaluate=true&lineWrap=true&presets=es2015%2Creact%2Cstage-2&targets=&version=6.26.0)

@sophiebits
Copy link
Contributor

We thought it would be good but then sorta never followed through. Now it seems more confusing than necessary.

@DanielRosenwasser
Copy link

TypeScript also doesn't support it. Perhaps it would be a good idea to revisit whether this should be in the spec at all. 😄

@Jessidhia
Copy link

Jessidhia commented Oct 13, 2017 via email

@loganfsmyth
Copy link

@clemmy Babel's parser has always supported it, but the transform itself either never worked or broke at some point. We've fixed it for 7.x babel/babel#6004

@sophiebits
Copy link
Contributor

Never worked. Can we hold off on adding this and make sure we get agreement that we want to turn it on?

@loganfsmyth
Copy link

If that's what people think, sure. I just took the fact that it's in the spec at face value and figured we should fix it since it was a trivial bug causing the breakage in the first place.

@sophiebits
Copy link
Contributor

It wasn't a bug, we had never gotten agreement to add it to the transform.

@loganfsmyth
Copy link

loganfsmyth commented Oct 13, 2017

Alright, good to know. It does seem super confusing that it's in the spec text if that is the case. Should we also remove it from the spec then? Or at least mark it as deprecated and not recommended.

@sebmarkbage
Copy link
Contributor

sebmarkbage commented Oct 13, 2017

I'm in favor of just adding it and starting to support it (if we have consensus).

@clemmy
Copy link
Contributor Author

clemmy commented Oct 13, 2017

Personally, I'm not the biggest fan of that syntax - it looks super confusing.

@DanielRosenwasser
Copy link

DanielRosenwasser commented Oct 14, 2017

I spoke with folks from our team. It does seem somewhat confusing, and clearly rare if nobody has filed an issue in all the time that Babel & TypeScript have supported JSX. We'd be for removing from the spec, or putting it on a deprecation path.

@RyanCavanaugh
Copy link

I raised this earlier at #53 without any success. We still haven't implemented elements in attribute positions in TypeScript and no one has cared. Given the lack of interest from anyone other than transpiler implementers it really seems like it ought to be removed before someone does implement it and cause a syntactic ambiguity that prevents more useful syntax from being added in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants