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

Proposal: throw syntax error for `}` and `>` in JSX text. #11042

Open
bradzacher opened this issue Jan 22, 2020 · 6 comments · Fixed by #11046
Open

Proposal: throw syntax error for `}` and `>` in JSX text. #11042

bradzacher opened this issue Jan 22, 2020 · 6 comments · Fixed by #11046

Comments

@bradzacher
Copy link

@bradzacher bradzacher commented Jan 22, 2020

Feature Request

  • I would like to work on this feature!
    • If needs be, I can look into it

Is your feature request related to a problem? Please describe.

The JSX spec [1][2] lists } and > as invalid JSX text characters.

JSXTextCharacter :

  • SourceCharacter but not one of {, <, > or }

However, babel does not throw a syntax error when it encounters a naked } or > in a JSXText string.

According to the spec, the only way you should be able to include either of these characters (similarly for { and <), is via a string expression: {'>'}.

Describe the solution you'd like
I propose that babel should follow the JSX spec, and throw a syntax error if it encounters a "naked" } or > within jsx text.

From my experience, it's uncommon that these characters are intentionally put in JSX text, and are instead the result of typos (i.e. typing {}, but having an editor auto-complete an extra } => {}}), or copy pasting errors.

Flow has already updated their parser to flag this as an error [3][4]. TypeScript is happy to make this change as well [5] (if Babel makes the change, as babel is generally considered the "reference implementation" for JSX).
I have also raised an issue in acorn for the same functionality [6].

Describe alternatives you've considered

  • Custom lint rules
    • We had these implemented at facebook before flow made it a parsing error. (The lint rules actually spurred the change).
    • Outside of a tightly controlled monorepo, I can see that it would be hard to gain adoption for the rule, as not everyone uses linting tools, not everyone uses the react plugins, etc etc.
  • Prettier auto-formatting to convert } to {'}'}.
    • This is an AST change, and prettier doesn't like to change the AST when formatting.

Teachability, Documentation, Adoption, Migration Strategy
If you can, explain how users will be able to use this and possibly write out a version the docs.
Maybe a screenshot or design?

I think the best way to document this would be via clear and explicit error messaging.
For example, the following code:

function F() {
  return <div>></div>;
}

function G() {
  return <div>{1}}</div>;
}

produces the following flow errors:

4:   return <div>></div>;
                 ^ Unexpected token `>`. Did you mean `{'>'}`?
8:   return <div>{1}}</div>;
                    ^ Unexpected token `}`. Did you mean `{'}'}`?

flow.org repl


Related:
[1] https://facebook.github.io/jsx/
[2] facebook/jsx#18
[3] https://github.com/facebook/flow/blob/master/Changelog.md#01160
[4] facebook/flow@e1d0038
[5] microsoft/TypeScript#36341
[6] acornjs/acorn-jsx#106

@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Jan 22, 2020

Hey @bradzacher! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Jan 22, 2020

Seems like a nice thing to add to Espree/Acorn (https://github.com/acornjs/acorn-jsx), as well!

@jridgewell

This comment has been minimized.

Copy link
Member

@jridgewell jridgewell commented Jan 22, 2020

This is a breaking change, so getting it in with Babel 8 would be good.

@bradzacher

This comment has been minimized.

Copy link
Author

@bradzacher bradzacher commented Jan 22, 2020

This is a breaking change, so getting it in with Babel 8 would be good.

Is there a timeline for the babel 8 release?
If it's soon, and there's limited bandwidth, I'd be happy to try and cobble together a PR.

@jridgewell

This comment has been minimized.

Copy link
Member

@jridgewell jridgewell commented Jan 22, 2020

We're just a few weeks away: #10746

This is actually a super easy fix, so it's a Good First Issue. Do you want to do it? The only changes necessary are in

if (!openingElement.selfClosing) {
contents: for (;;) {
switch (this.state.type) {
case tt.jsxTagStart:
startPos = this.state.start;
startLoc = this.state.startLoc;
this.next();
if (this.eat(tt.slash)) {
closingElement = this.jsxParseClosingElementAt(
startPos,
startLoc,
);
break contents;
}
children.push(this.jsxParseElementAt(startPos, startLoc));
break;
case tt.jsxText:
children.push(this.parseExprAtom());
break;
case tt.braceL: {
const node = this.startNode();
this.next();
if (this.match(tt.ellipsis)) {
children.push(this.jsxParseSpreadChild(node));
} else {
children.push(this.jsxParseExpressionContainer(node));
}
break;
}
// istanbul ignore next - should never happen
default:
throw this.unexpected();
}
}
, and some tests.

@bradzacher

This comment has been minimized.

Copy link
Author

@bradzacher bradzacher commented Jan 22, 2020

Sure thing, I'll take a look at it in the next few days.
I'd love to get this in for babel 8, as I think it's a huge win.

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

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.