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

Add early error to disallow invalid adjacent JSX elements #120

Open
nicolo-ribaudo opened this issue Sep 2, 2019 · 3 comments · May be fixed by #141
Open

Add early error to disallow invalid adjacent JSX elements #120

nicolo-ribaudo opened this issue Sep 2, 2019 · 3 comments · May be fixed by #141
Labels
Impl Reality Reality that the spec does not capture Normative

Comments

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Sep 2, 2019

Currently this is perfectly valid JSX code:

<El>text</El> <g> / </g>
0

How is it possible? Adjacent JSX elemets must be wrapped inside another element, or inside a fragment. Well, those are not two JSX elements.

It's a jsx element (<El>text</El>), which is compared by < with the variable g, which is then compared by > with the regex / </g, and which is then compared by > with 0.
If we add some parentheses for readability, it becomes

(((<El>text</El>) < g) > / </g) > 0

Currently JSX parsers don't handle this (valid) code well, because they all assume that the user is trying to use to JSX elements:

I propose to disallow that code since it is highly confusing, similarly to how -1 ** 2 is disallowed by the ECMAScript specification. To do so, we would need to define a new early error which applies to the RelationalExpression production:

It is an early Syntax Error if RelationalExpression matches the JSXElement < ShiftExpression or JSXFragment < ShiftExpression productions

By doing so, the spec would align with the ecosystem and allow parsers to throw errors helpful for the users, like Babel's Adjacent JSX elements must be wrapped in an enclosing tag. Did you want a JSX fragment <>...</>?

@Huxpro Huxpro added Impl Reality Reality that the spec does not capture Normative labels Feb 25, 2022
@Huxpro
Copy link
Contributor

Huxpro commented Feb 25, 2022

@nicolo-ribaudo Oh wow this is interesting. Thanks for reporting this and providing a very clear analysis.

Since we've moved the spec to ecmarkup (#135), I think we are finally in a good position to formalize errors like this. Would you mind making a PR?

Regarding the potential breaking change on the Flow parser: @pieterv, is Flow team comfortable with this change?

@nicolo-ribaudo nicolo-ribaudo linked a pull request Feb 25, 2022 that will close this issue
@pieterv
Copy link
Member

pieterv commented Mar 1, 2022

Looks like this has been fixed in Flow since this issue has been created as the astexplorer link is showing the elements after the first JSX element parsed as binary expressions. Seems like a reasonable change anyway, we can fix any issues that come up.

See: https://astexplorer.net/#/gist/66ddd5a43c135ee13b7872e65de033c4/47ffc8a8b45cbbc6e8152423b9c7d5c351934288

@Huxpro
Copy link
Contributor

Huxpro commented Mar 7, 2022

@nicolo-ribaudo

I propose to disallow that code since it is highly confusing, similarly to how -1 ** 2 is disallowed by the ECMAScript specification.

How is this specified? I couldn't find any SS: early errors clause dedicated for that in the full spec or the original https://tc39.es/proposal-exponentiation-operator/ addition. I was imaging that we can specify JSXElement < similarly in #141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impl Reality Reality that the spec does not capture Normative
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants