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

Fix prettier test compat with Babel 8 #14612

Closed
wants to merge 3 commits into from

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 28, 2022

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This tests contains invalid JSX that is disallowed in Babel 8. https://github.com/prettier/prettier/blob/main/tests/format/typescript/optional-variance/with-jsx.tsx

@babel-bot
Copy link
Collaborator

babel-bot commented May 28, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52067/

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review May 28, 2022 19:32
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented May 28, 2022

This fixes the E2E breaking prettier test. The remaining failures are fixed by #14592.

cc @sosukesuzuki I would love your review, since you know how prettier tests work! Also, I wonder if prettier could format > in JSX as {">"}, since > is technically invalid.

@liuxingbaoyu
Copy link
Member

Is this a temporary fix?
This test also exists in our repo, just with babel8 disabled.

@nicolo-ribaudo
Copy link
Member Author

No, this is not temporary! JSX text cannot contain >, but we are disallowing it only in Babel 8 to avoid breaking changes. JSX has a test where it uses > in JSX, so it doesn't work with Babel 8: this PR updates that test to make it valid.

@nicolo-ribaudo
Copy link
Member Author

#12451

@liuxingbaoyu
Copy link
Member

The only change is that I made the error recoverable (just by deleting throw in front of this.raise).

parseMaybeAssign(...args): N.Expression {
// Note: When the JSX plugin is on, type assertions (`<T> x`) aren't valid syntax.
let state: ?State;
let jsx;
let typeCast;
if (
this.hasPlugin("jsx") &&
(this.match(tt.jsxTagStart) || this.match(tt.lt))
) {
// Prefer to parse JSX if possible. But may be an arrow fn.
state = this.state.clone();
jsx = this.tryParse(() => super.parseMaybeAssign(...args), state);
/*:: invariant(!jsx.aborted) */
/*:: invariant(jsx.node != null) */
if (!jsx.error) return jsx.node;

It seems we can modify this to make it recoverable.
Instead of the current behavior SyntaxError: Unexpected token.

@nicolo-ribaudo
Copy link
Member Author

I opened #14616 to make it recoverable, which is way better. Thanks! 😄

@nicolo-ribaudo nicolo-ribaudo deleted the fix-prettier-e2e branch May 29, 2022 21:37
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants