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 start position of intersection/union node #31017
Conversation
let type = parseConstituentType(); | ||
const startPos = leadingToken ? leadingToken.pos : type.pos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (token() === operator) { | ||
const types = [type]; | ||
while (parseOptional(operator)) { | ||
types.push(parseConstituentType()); | ||
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
parseOptionalToken
usescanner.getStartPos()
as start position. What about usescanner.getTokenPos()
instead?