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: don't deduplicate comments with same start index #13169
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45409/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 70e0c2b:
|
@@ -348,6 +348,20 @@ describe("generation", function () { | |||
const output = generate(type).code; | |||
expect(output).toBe("(infer T)[]"); | |||
}); | |||
|
|||
it("comments with same start index", () => { | |||
const code1 = "/* leading a */ a();"; |
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.
The example of generating multiple statements for the program body in the PR author comment seems like a more likely scenario than what we're testing here. Can we change this to match that?
Since modifying anything with comments ends up affecting a rather large surface area, I wonder if it would be prudent to split this up into two PRs (the cloning fix in one and the deduplication fix in the other) in case we find regressions and need to revert something. |
f26659f
to
760ab52
Compare
@kaicataldo Good point, the cloning fix was moved to #13178. |
760ab52
to
70e0c2b
Compare
I think the check that is removed here was added so that the comment in this example (which exists twice in the AST, both as leading and trailing) if (1) {
a();
}
// /* */
else {
b();
} isn't printed twice. Have you checked that this case still behaves correctly? |
@mischnic We already have a similar test case: babel-generator/test/fixtures/comments/try-block-line-comment/input.js |
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 that the original use case for this object is now better handled by the _printedComments
WeakSet
.
Input code
Expected behavior
Current behavior
This PR includes following changes: