Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix moving comments of removed nodes #15427

Merged
merged 5 commits into from
Mar 30, 2023
Merged

Fix moving comments of removed nodes #15427

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5948063
Fix moving comments of removed nodes
nicolo-ribaudo Feb 15, 2023
e260f1f
Update other fixtures
nicolo-ribaudo Feb 15, 2023
fb76e44
Remove comment
nicolo-ribaudo Feb 15, 2023
af1d8a9
Restore behavior of class props comments
nicolo-ribaudo Feb 15, 2023
47f4793
Update fixtures
nicolo-ribaudo Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ugin-proposal-class-properties/test/fixtures/compile-to-class/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ class C {
/* before a */
babelHelpers.defineProperty(this, "a", void 0);
}
/* after a */
/* before b */
/* after b */
}
/* after a */
/* before b */
babelHelpers.defineProperty(C, "b", void 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO in this case the old behaviour is more preferable, /* before b */ is more likely to be the JSDoc comment of class property b.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think should be the priority? When we remove node B from ABC, we first try to attach B's trailing comments as leading comments of C, and then fallback to trailing comments of A?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok actually, the priority I implemented is already like that.

What's happening here is that when we have this code:

class C {
  constructor() {}
  /* before b */
  static b;
  /* after b */
}

we call path.remove() on static b, and thus its comments are attached to constructor() {} which is the only available sibling node.

I'm confused by the previous output, because after b was attached to the constructor but before b wasn't 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a solution, I'm not sure I like it. The behavior I was confused about was because of the helpers for class fields to inherit comments that we recently introduced.

4 changes: 3 additions & 1 deletion ...-plugin-proposal-class-properties/test/fixtures/private-loose/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ var C = /*#__PURE__*/babelHelpers.createClass(function C() {
writable: true,
value: void 0
});
} /* after b */);
}
/* after a */
/* before b */
/* after b */
);
Object.defineProperty(C, _b, {
writable: true,
value: void 0
Expand Down
4 changes: 3 additions & 1 deletion .../babel-plugin-proposal-class-properties/test/fixtures/private/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ var C = /*#__PURE__*/babelHelpers.createClass(function C() {
writable: true,
value: void 0
});
} /* after b */);
}
/* after a */
/* before b */
/* after b */
);
var _b = {
writable: true,
value: void 0
Expand Down
4 changes: 3 additions & 1 deletion ...l-plugin-proposal-class-properties/test/fixtures/public-loose/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ var C = /*#__PURE__*/babelHelpers.createClass(function C() {
babelHelpers.classCallCheck(this, C);
/* before a */
this.a = void 0;
} /* after b */);
}
/* after a */
/* before b */
/* after b */
);
C.b = void 0;
4 changes: 3 additions & 1 deletion ...s/babel-plugin-proposal-class-properties/test/fixtures/public/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ var C = /*#__PURE__*/babelHelpers.createClass(function C() {
babelHelpers.classCallCheck(this, C);
/* before a */
babelHelpers.defineProperty(this, "a", void 0);
} /* after b */);
}
/* after a */
/* before b */
/* after b */
);
babelHelpers.defineProperty(C, "b", void 0);
4 changes: 2 additions & 2 deletions ...plugin-proposal-private-methods/test/fixtures/accessors-loose/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ class C {
set: _set_a
});
}
/* after get a */
/* before set a */
/* after set a */
}
function _get_a() {
return 42;
}
/* after get a */
/* before set a */
function _set_a(v) {}
4 changes: 2 additions & 2 deletions ...ate-methods/test/fixtures/accessors-privateFieldsAsProperties/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ class C {
set: _set_a
});
}
/* after get a */
/* before set a */
/* after set a */
}
function _get_a() {
return 42;
}
/* after get a */
/* before set a */
function _set_a(v) {}
4 changes: 2 additions & 2 deletions ...babel-plugin-proposal-private-methods/test/fixtures/accessors/preserve-comments/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ class C {
set: _set_a
});
}
/* after get a */
/* before set a */
/* after set a */
}
function _get_a() {
return 42;
}
/* after get a */
/* before set a */
function _set_a(v) {}
3 changes: 3 additions & 0 deletions .../babel-plugin-transform-new-target/test/fixtures/general/class-properties-loose/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ class Foo {
// should not replace
this.q = this.constructor;
} // should not replace
// should not replace
// should replace
// should not replace
}, _class.p = void 0, _class.p1 = class {
constructor() {
this.constructor;
Expand Down
3 changes: 3 additions & 0 deletions packages/babel-plugin-transform-new-target/test/fixtures/general/class-properties/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ class Foo {
// should not replace
babelHelpers.defineProperty(this, "q", this.constructor);
} // should not replace
// should not replace
// should replace
// should not replace
}, babelHelpers.defineProperty(_class, "p", void 0), babelHelpers.defineProperty(_class, "p1", class {
constructor() {
this.constructor;
Expand Down
4 changes: 3 additions & 1 deletion packages/babel-plugin-transform-typescript/test/fixtures/exports/declared-types/output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
export class C2 {}

// Not-even E

// everything removed
export { C2 as C3 }; // only C2->C3
var BB;
Expand All @@ -18,6 +21,5 @@ function foo() {}
export { BB2 as BB3, foo }; // only BB2->BB3 and foo

// export an interface before declaration

// everything removed
export { C2 as C4 }; // only C2->C4
17 changes: 17 additions & 0 deletions packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @module
*/

import '../message-bus';

/**
* @typedef {Function} PromiseResolveCb
*/
export type PromiseResolveCb<T> = (value?: T | PromiseLike<T>) => void;

/**
* @typedef {Function} PromiseRejectCb
*/
export type PromiseRejectCb = (reason?: any) => void;

var a;
15 changes: 15 additions & 0 deletions packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* @module
*/

import '../message-bus';

/**
* @typedef {Function} PromiseResolveCb
*/

/**
* @typedef {Function} PromiseRejectCb
*/

var a;
15 changes: 15 additions & 0 deletions packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* @module
*/

import '../message-bus';

/**
* @typedef {Function} PromiseResolveCb
*/
export type PromiseResolveCb<T> = (value?: T | PromiseLike<T>) => void;

/**
* @typedef {Function} PromiseRejectCb
*/
export type PromiseRejectCb = (reason?: any) => void;
13 changes: 13 additions & 0 deletions packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* @module
*/

import '../message-bus';

/**
* @typedef {Function} PromiseResolveCb
*/

/**
* @typedef {Function} PromiseRejectCb
*/
15 changes: 15 additions & 0 deletions packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* @module
*/

/**
* @typedef {Function} PromiseResolveCb
*/
export type PromiseResolveCb<T> = (value?: T | PromiseLike<T>) => void;

/**
* @typedef {Function} PromiseRejectCb
*/
export type PromiseRejectCb = (reason?: any) => void;

var a;
14 changes: 14 additions & 0 deletions packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* @module
*/

/**
* @typedef {Function} PromiseResolveCb
*/

/**
* @typedef {Function} PromiseRejectCb
*/

var a;
export {};
32 changes: 28 additions & 4 deletions packages/babel-traverse/src/path/comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,37 @@ export function shareCommentsWithSiblings(this: NodePath) {
const next = this.getSibling(this.key + 1);
const hasPrev = Boolean(prev.node);
const hasNext = Boolean(next.node);
if (hasPrev && !hasNext) {
prev.addComments("trailing", trailing);
} else if (hasNext && !hasPrev) {
next.addComments("leading", leading);

if (hasPrev) {
if (leading) {
prev.addComments(
"trailing",
removeIfExisting(leading, prev.node.trailingComments),
);
}
if (trailing && !hasNext) prev.addComments("trailing", trailing);
}
if (hasNext) {
if (trailing) {
next.addComments(
"leading",
removeIfExisting(trailing, next.node.leadingComments),
);
}
if (leading && !hasPrev) next.addComments("leading", leading);
}
}

function removeIfExisting<T>(list: T[], toRemove?: T[]): T[] {
if (!toRemove) return list;
let lastFoundIndex = -1;
return list.filter(el => {
const i = toRemove.indexOf(el, lastFoundIndex);
if (i === -1) return true;
lastFoundIndex = i;
});
}

export function addComment(
this: NodePath,
type: t.CommentTypeShorthand,
Expand Down
5 changes: 2 additions & 3 deletions packages/babel-traverse/test/fixtures/comments/attachment/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
{
hasPrev;
/* top */
/* left */ /* right */
/* left */
/* right */
/* bottom */
}

{
/* top */
/* left */ /* right */
Expand All @@ -16,7 +16,6 @@
hasPrev;
/* top */
/* left */

/* right */
/* bottom */
hasNext;
Expand Down