Fix start position of intersection/union node#31017
Conversation
| parseOptional(operator); | ||
| const leadingToken = parseOptionalToken(operator); | ||
| let type = parseConstituentType(); | ||
| const startPos = leadingToken ? leadingToken.pos : type.pos; |
There was a problem hiding this comment.
Can you add a test for this output. It seems like this change would result in missing comments:
type A =
/*leading0*/
| /*leading1*/ 1 /*trailing1*/
| /*leading2*/ 2 /*trailing2*/;When we emit this declaration today, we include the leading and trailing comments for each type in the union, but do not emit /*leading0*/. I would like to see the test verify that we still preserve the comments around each type and whether this will now also include the leading comment before the first |.
There was a problem hiding this comment.
Also, I would move this line inside of the if statement that follows it? There is no need to determine startPos if we end up not using it.
| } | ||
| const node = <UnionOrIntersectionTypeNode>createNode(kind, type.pos); | ||
| const node = <UnionOrIntersectionTypeNode>createNode(kind, startPos); | ||
| node.types = createNodeArray(types, type.pos); |
There was a problem hiding this comment.
should the NodeArray also include the leading operator? currently only the Union/IntersectionType contains the operator
Have a look at my attempt: #31265 For the test @rbuckton requested:
In my PR (linked above) I used |
|
Hi. There is a discussion about how this should be fixed on the original issue #30995. Especially, this PR leaves the leading operator as floating if the second operator didn't exist. |
Fixes #30995
parseOptionalTokenusescanner.getStartPos()as start position. What about usescanner.getTokenPos()instead?