Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fixed declaration emit issue related to a qualifier being reused cross-file #58810
Fixed declaration emit issue related to a qualifier being reused cross-file #58810
Changes from 2 commits
c51749a
68a0fcf
2a217f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Some other code paths in this function do a similar "cloning" of the
.questionToken
so I assume that it's the right thing to do here and in other places that I touchedThere 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.
Those 2 call sites related to
.questionToken
are also related to JSDoc. I wonder if this cloning even matters in this context.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.
those modifiers are just tokens so i guess i could just map over them and clone them?
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.
those are not jus flat tokens so I wasn't quite sure what to do about those, it seemed like maybe i should deep clone them?
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.
I definitely don't have enough context on the workings of the checker to give any informed feedback, but I can at least ask at a high level do we have some confidence whether anything more could be done to the structure of the code to avoid this general class of problem?
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.
Tbh, I don't know this part of TS too well either so we need to wait for @weswigham or @dragomirtitian to give some feedback on this. I'd expect that other things in this function (like
node.attributes
here) should be wrapped like this too (if only this is the right fix).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.
I would definitely suspect a lot of this needs this same fix.
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.
100% - any child node we copy should have its' children visited - any missing visitor calls on manually remapped nodes are an oversight, presumably.
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.
there are also places in this function that do smth like:
this pattern is not applied consistently across the whole function, should it be? 🤔
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.
I suspect yes.... Sure seems like though that we should make some sort of wrapper for the visitor func that would check that. Though I feel like I've seen that exact code in some visitor transformer code already (maybe we're not going through that)
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.
I think this might be a bigger change so I'd leave it out of this PR. I just want to flag this as a potential problem.