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

[ts 4.7] Support optional variance annotations #14359

Merged
merged 9 commits into from May 17, 2022

Conversation

magic-akari
Copy link
Contributor

@magic-akari magic-akari commented Mar 15, 2022

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

This PR Implement Optional variance annotations

@magic-akari magic-akari force-pushed the feat/ts-VarianceAnnotations branch from 8217086 to ddc567d Compare Mar 15, 2022
@magic-akari magic-akari force-pushed the feat/ts-VarianceAnnotations branch from 559c4a3 to c300420 Compare Mar 16, 2022
@magic-akari magic-akari force-pushed the feat/ts-VarianceAnnotations branch from c300420 to c6db874 Compare Mar 16, 2022
@magic-akari magic-akari marked this pull request as ready for review Mar 16, 2022
Copy link
Contributor

@JLHwung JLHwung left a comment

Can you add some test cases?

// with plugins `jsx` and `typescript`
<in T>() => {}

It should throw SingleTypeParameterWithoutTrailingComma error.

And also a type-variance-like JSX element:

<in T>() => {}</in>

It should be parsed successfully.

this.word("out");
this.space();
}

Copy link
Contributor

@JLHwung JLHwung Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a generator test case to packages/babel-generator/test/fixtures/typescript?

Copy link
Contributor

@liuxingbaoyu liuxingbaoyu May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm investigating ci failures in main branch, but I don't know much about ts and jsx.
<in T>() => {}</in> will fail when BABEL_8_BREAKING is enabled.
Can someone tell me if this is a problem with the parser or with the test?

@JLHwung JLHwung added PR: New Feature 🚀 area: typescript labels Mar 16, 2022
@magic-akari
Copy link
Contributor Author

@magic-akari magic-akari commented Mar 17, 2022

Test Added. please do not merge until microsoft/TypeScript#48240 is merged.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.18.0 milestone Mar 22, 2022
@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged label Mar 22, 2022
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 22, 2022

cc @bradzacher if you have any comment regarding the AST shape

in?: boolean,
out?: boolean,
Copy link
Contributor

@bradzacher bradzacher Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sold on using boolean flags as it locks us in with naming but I guess that this is the same thing we did with class property modifiers as well so it's not like we're breaking convention in the AST.

Given the precedence - this is probably the best approach, so LGTM

@magic-akari
Copy link
Contributor Author

@magic-akari magic-akari commented Apr 1, 2022

@JLHwung
Variance annotations is not supported on generic function type parameters.
What should we do?

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 1, 2022

@magic-akari I think the AST shape is fine. We can add a allowedModifiers param to tsParseTypeParameter and handle it accordingly for every tsParseTypeParameter usage.

@JLHwung JLHwung self-requested a review Apr 1, 2022
@nicolo-ribaudo nicolo-ribaudo changed the title feat: support optional variance annotations [ts 4.7] Support optional variance annotations May 17, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 6415f09 into babel:main May 17, 2022
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: typescript PR: New Feature 🚀 PR: Ready to be Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants