search

acornjs / acorn-jsx

Alternative, faster React.js JSX parser
MIT License
648 stars 72 forks source link

Throw unexpected token error for } and > in JSXText nodes #108

Closed kaicataldo closed 4 years ago

kaicataldo commented 4 years ago

Fixes https://github.com/acornjs/acorn-jsx/issues/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.

RReverser commented 4 years ago

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).

kaicataldo commented 4 years ago

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.

RReverser commented 4 years ago

@kaicataldo I wonder if it would make sense to:

  1. Land this in Acorn JSX under an option (disabled by default).
  2. Land this in major ESLint release with that option enabled by default.
  3. Wait couple of months (?) until projects update.
  4. Flip the default in major Acorn JSX release.

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.

kaicataldo commented 4 years ago

Either of those options sounds good to me! What would you prefer?

kaicataldo commented 4 years ago

Just wanted to follow up to see how you'd like to proceed. Thanks!

RReverser commented 4 years ago

I suppose we can even publish this as 5.2.0 - technically it's a minor breaking change, and semver-wise this would still prevent accidental automatic upgrade.

kaicataldo commented 4 years ago

That makes sense to me. Is there anything else you need from me before merging this?

RReverser commented 4 years ago

No, sorry, just been busy. I'll try to get to this on the weekend.

kaicataldo commented 4 years ago

No worries! Just wanted to make sure I wasn't holding anything up :)

RReverser commented 4 years ago

Apologies for the delay, and thanks again for the PR! Released as 5.2.0.

kaicataldo commented 4 years ago

No worries! Thanks for maintaining this project :)

ljharb commented 4 years ago

This minor release did, indeed, break eslint-plugin-react: https://github.com/yannickcr/eslint-plugin-react/pull/2583. There are no "minor" breaking changes :-(

RReverser commented 4 years ago

@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.