fix: Missing parentheses when printing a TS arrow function type in a union#17125
fix: Missing parentheses when printing a TS arrow function type in a union#17125JLHwung merged 4 commits intobabel:mainfrom
Conversation
liuxingbaoyu
commented
Feb 10, 2025
| Q | A |
|---|---|
| Fixed Issues? | Fixes #17123 |
| Patch: Bug Fix? | √ |
| Major: Breaking Change? | |
| Minor: New Feature? | |
| Tests Added + Pass? | √ |
| Documentation PR Link | |
| Any Dependency Changes? | |
| License | MIT |
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58702 |
|
Can you also add other test cases for arrow types, and check that they work? type t = (() => a) extends b ? c : d;
type t = a extends (() => b) ? c : d;
type t = a extends b ? () => c : d;
type t = a extends b ? c : () => d;
type t = (() => a)[];
type t = (() => a)["name"]; |
|
Amazing! |
There was a problem hiding this comment.
Essentially we should handle everything descended from tsParseType all the way until tsParseNonArrayType such that the TSFunctionalType may be created at the start position of the tsParseType.
tsParseType: TSConditionalType (checkType, extendsType)
tsParseNonConditionalType:
tsParseUnionTypeOrHigher: TSUnionType (n)
tsParseIntersectionTypeOrHigher: TSIntersectionType (n)
tsParseTypeOperatorOrHigher:
tsParseArrayTypeOrHigher: TSArrayType, TSIndexedAccessType (objectType)
tsParseNonArrayType
n: Extra rules also forbid b | () => c and a & () => c though they are perfectly unambiguous.
This route is also required because it does not cross any tsParseType boundary and the optional type is a postfix operator such that TSFunctionalType may be created at the start:
tsParseNonArrayType:
tsParseTupleType:
tsParseTupleElementType: TSOptionalType
tsParseNonArrayType
So we should further check the objectType key for TSIndexedAccessType although T[() => string] does not have much practical usage.
| (parent.checkType === node || parent.extendsType === node)) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
We should handle TSConstructorType such as new () => Date or abstract new () => Date as well.
There was a problem hiding this comment.
It seems like this doesn't need the parentheses?
There was a problem hiding this comment.
Well in this case the assignment expression lhs = rhs must not be a TSType so () => void = x is not a TSType, therefore parentheses is not required here. But we definitely need to disambiguate between abstract new () => number[] and (abstract new () => number)[], as well as optional type, intersection, ... etc.
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
#17125 (comment) has been done, right?
(I'm eager to release this, because it's being very annoying in my experiments with publishing to JSR as I currently have to manually edit the build output 😛)
| parentType === "TSUnionType" || | ||
| parentType === "TSOptionalType" || | ||
| parentType === "TSArrayType" || | ||
| parentType === "TSConstructorType" || |
There was a problem hiding this comment.
I don't think this check is required, the following examples are equivalent:
type T = () => abstract new () => string
type T = () => (abstract new () => string)…union (babel#17125) * fix * review * review * review