Skip to content

fix: don't deduplicate comments with same start index#13169

Merged
nicolo-ribaudo merged 1 commit intobabel:mainfrom
gzzhanghao:fx/duplicate-comment
Apr 21, 2021
Merged

fix: don't deduplicate comments with same start index#13169
nicolo-ribaudo merged 1 commit intobabel:mainfrom
gzzhanghao:fx/duplicate-comment

Conversation

@gzzhanghao
Copy link
Copy Markdown
Contributor

@gzzhanghao gzzhanghao commented Apr 18, 2021 •
edited
Loading

Q                       A
Fixed Issues? Fixes #12769
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link No
Any Dependency Changes? No
License MIT

Input code

const template = require('@babel/template');
const generator = require('@babel/generator');
const t = require("@babel/types");

const createStatement = template.statement("/*#__PURE__*/ foo()", {
  preserveComments: true,
});

const result = t.program([createStatement(), createStatement()]);

console.log(generator.default(result).code);

Expected behavior

/*#__PURE__*/
foo();
/*#__PURE__*/
foo();

Current behavior

/*#__PURE__*/
foo();
foo();

This PR includes following changes:

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Apr 18, 2021 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45409/

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Apr 18, 2021 •
edited
Loading

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:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@gzzhanghao gzzhanghao changed the title fix: generate comments with same start index (#12769) fix: generate comments with same source Apr 18, 2021
kaicataldo
kaicataldo reviewed Apr 19, 2021
View reviewed changes
packages/babel-generator/test/index.js Outdated
});

it("comments with same start index", () => {
const code1 = "/* leading a */ a();";
Copy link
Copy Markdown
Member

@kaicataldo kaicataldo Apr 19, 2021

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?

@kaicataldo
Copy link
Copy Markdown
Member

kaicataldo commented Apr 19, 2021

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.

@kaicataldo kaicataldo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: generator pkg: types labels Apr 19, 2021
@kaicataldo kaicataldo mentioned this pull request Apr 19, 2021
Closed
1 task
@gzzhanghao gzzhanghao force-pushed the fx/duplicate-comment branch from f26659f to 760ab52 Compare April 20, 2021 05:14
@gzzhanghao gzzhanghao closed this Apr 20, 2021
@gzzhanghao gzzhanghao deleted the fx/duplicate-comment branch April 20, 2021 05:15
@gzzhanghao gzzhanghao restored the fx/duplicate-comment branch April 20, 2021 05:16
@gzzhanghao gzzhanghao reopened this Apr 20, 2021
@gzzhanghao gzzhanghao mentioned this pull request Apr 20, 2021
Merged
@gzzhanghao
Copy link
Copy Markdown
Contributor Author

gzzhanghao commented Apr 20, 2021

@kaicataldo Good point, the cloning fix was moved to #13178.

@gzzhanghao gzzhanghao changed the title fix: generate comments with same source fix: don't deduplicate comments with same start index Apr 20, 2021
@gzzhanghao gzzhanghao force-pushed the fx/duplicate-comment branch from 760ab52 to 70e0c2b Compare April 20, 2021 05:27
@mischnic
Copy link
Copy Markdown
Contributor

mischnic commented Apr 20, 2021

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?

@gzzhanghao
Copy link
Copy Markdown
Contributor Author

gzzhanghao commented Apr 20, 2021

@mischnic We already have a similar test case: babel-generator/test/fixtures/comments/try-block-line-comment/input.js

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Apr 20, 2021

Yeah, @mischnic's code works in the REPL for this PR! https://babeljs.io/repl/build/45409/#?browsers=chrome%2060&build=&builtIns=false&spec=false&loose=false&code_lz=JYMwBAFAjAlGDeAoMYCGEYG5EF9EHp8x8AqME_RAUwBsBnKhZMAIw2xyA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=&prettier=false&targets=&version=7.13.15%2Bpr.13169&externalPlugins=

nicolo-ribaudo
nicolo-ribaudo approved these changes Apr 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a 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.

@nicolo-ribaudo nicolo-ribaudo merged commit 66181db into babel:main Apr 21, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kaicataldo kaicataldo kaicataldo left review comments

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

@babel/generator comment deduplication removes too many comments

6 participants

@gzzhanghao @babel-bot @kaicataldo @mischnic @nicolo-ribaudo @JLHwung