Throw unexpected token error for } and > in JSXText nodes #108
Conversation
test/tests-jsx.js
Outdated
| } | ||
| } | ||
| }, | ||
| // '<LeftRight left=<a /> right=<b>monkeys /> gorillas</b> />': { |
There was a problem hiding this comment.
Thoughts on what to do with this? It looks like this this is still in the test suite.
There was a problem hiding this comment.
It looks like this was altered to the correct form in the corresponding PR to babel. Should we do the same here?
There was a problem hiding this comment.
I've updated this to include valid as well as invalid variants of this.
test/tests-jsx.js
Outdated
| @@ -3958,3 +3958,657 @@ for (var ns in fbTestFixture) { | |||
| }); | |||
| } | |||
| } | |||
|
|
|||
| testFail("<LeftRight left=<a /> right=<b>monkeys /> gorillas</b> />", 'Unexpected token. Did you mean {">"} instead? (1:40)') | |||
| testFail("<A>foo{</A>", "Unexpected token (1:8)") | |||
There was a problem hiding this comment.
These already throw an error.
test/tests-jsx.js
Outdated
|
|
||
| testFail("<LeftRight left=<a /> right=<b>monkeys /> gorillas</b> />", 'Unexpected token. Did you mean {">"} instead? (1:40)') | ||
| testFail("<A>foo{</A>", "Unexpected token (1:8)") | ||
| testFail("<A>foo<</A>", "Unexpected token (1:7)") |
There was a problem hiding this comment.
These already throw an error.
|
Thanks for the PR. I'm surprised this was not noticed for literally years since the change to spec was made... On one hand, it seems reasonable to fix it, but on another, I'm worried about breaking backwards compatibility in case there are projects that depend on this (and it seems all of the tools happily accepted these chars before). |
|
Thanks for the review! I believe both Babel and TypeScript are adding this error, and ESLint is currently working on a major release in which we could add this breaking change. Another option on the ESLint side of things would be to check this in Espree, though I’m not sure of the performance implications of that, and it does feel to me like this should be in this plugin. Totally understand where you’re coming from, though! If this isn’t likely to land, I can explore the alternative I laid out above. |
|
@kaicataldo I wonder if it would make sense to:
UPD: Or, I guess, we could just ship this in a major Acorn JSX now, given that there is little or no planned maintenance for the current major version. |
|
Either of those options sounds good to me! What would you prefer? |
kaicataldo
force-pushed
the
disallow-jsxtext-tokens
branch
from
976d571
to
88ffffc
Compare
kaicataldo
force-pushed
the
disallow-jsxtext-tokens
branch
from
88ffffc
to
a5ebc8e
Compare
kaicataldo
force-pushed
the
disallow-jsxtext-tokens
branch
from
b09b210
to
b8654eb
Compare
kaicataldo
force-pushed
the
disallow-jsxtext-tokens
branch
from
08ac2f8
to
71541e5
Compare
|
Just wanted to follow up to see how you'd like to proceed. Thanks! |
|
I suppose we can even publish this as |
|
That makes sense to me. Is there anything else you need from me before merging this? |
|
No, sorry, just been busy. I'll try to get to this on the weekend. |
|
No worries! Just wanted to make sure I wasn't holding anything up :) |
|
Apologies for the delay, and thanks again for the PR! Released as 5.2.0. |
|
No worries! Thanks for maintaining this project :) |
|
This minor release did, indeed, break eslint-plugin-react: jsx-eslint/eslint-plugin-react#2583. There are no "minor" breaking changes :-( |
|
@ljharb I'm sorry for that :( This was a tough choice with unclear fallout due to it being, in theory, a bugfix to what should've never worked. I suppose unpublishing at this point might equally break users who started relying on the new version, so will have to leave as-is. |
Fixes #106
Thoughts on the error message? The corresponding PR to babel also lists the HTML entity.
This is my first time contributing to this repository, so please let me know if I should be doing anything differently.