Repository: babel/babel (TypeScript)

Issue:
@babel/plugin-transform-typescript strips vital JSDoc @typedef comments
## Bug Report

<br>

**EDIT:** See my comment https://github.com/babel/babel/issues/9916#issuecomment-489404010 below, there is a strange inconsistency depending on the location of `import` statements.

<br>

Running Babel with nothing but @babel/plugin-transform-typescript removes all but the very first of many JSDoc `@typedef` comments.

Those comments are for (and just above) Typescript type definitions (export type, export interface). When the Typescript types are stripped, so are the JSDoc comments. When I run `jsdoc3` to generate the HTML API docs this tool too runs Babel and this plugin in order to be able to parse the file. Obviously, the removal of the JSDoc is counterproductive especially in this use case.


**Environment**
- Babel version(s): 7.4.4
- Node/npm version: 11.14
- jsdoc 3.5.5
- OS: Linux

- How you are using Babel: jsdoc3, config (jsdoc.json, babel section):

```
"babel": {
        "extensions": ["js","ts","jsx","tsx"],
        "presets": ["@babel/preset-typescript"],
        "plugins": [],
        "babelrc": false
    },
```


## Example

Here is an excerpt of the beginning of a file with many TS type definitions and there accompanying JSDoc comments.

```js
/**
 * This defines the creation status string constants for files
 * @global
 * @typedef {("new"|"exists")} FileCreationStatus
 */
export type FileCreationStatus = typeof CREATION_STATUS[keyof typeof CREATION_STATUS];

/**
 * @global
 * @typedef {Object} FileCreation
 * @property {SHA256Hash} hash - The SHA-256 hash of the contents of a versioned ONE object
 * @property {FileCreationStatus} status - A string constant showing whether the file
 * already existed or if it had to be created.
 */
export interface FileCreation {
    hash: SHA256Hash;
    status: FileCreationStatus;
}

/**
 * This defines the creation status string constants for objects.
 * @global
 * @typedef {FileCreationStatus} ObjectCreationStatus
 */
export type ObjectCreationStatus = FileCreationStatus;
```
After the transformation only this is left of these comments:

```js
/**
 * This defines the creation status string constants for files
 * @global
 * @typedef {("new"|"exists")} FileCreationStatus
 */
export type FileCreationStatus = typeof CREATION_STATUS[keyof typeof CREATION_STATUS];
```

Lots and lots of comments erased while there were nothing but Typescript type definitions encountered by the parser. Only when there was actual JS code again did the comment removal stop.

## Proposed solution

Don't delete any JSDoc (or any) comments. Comment removal should be left to the boolean "comment" Babel configuration property and whatever Babel code is following that instruction. If it is not set to `true` no comments should be removed, especially not JSDoc `@typedef` ones.



Hints:
Hey @lll000111! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant [Slack
community](https://babeljs.slack.com) that typically always has someone willing to help. You can sign-up [here](https://slack.babeljs.io/)
for an invite.
Could someone give me a hint, please?

I tried changing the code myself. Whenever I try to do `node.replaceWith` it won't let me, telling me "[thisSameThing] expecte".

I want to either just replace the nodes with the comments, or with some other node (one that JSDoc ignores). I can't replace anything. And just commenting out various `remove()` statements worked at first, so after producing the JSDoc I put everything back into the original state because it was so easy and I need the removal for the regular build. But when I try it now that same simple commenting out leads to JSDoc parse errors. So now I have to actually replace those comments.

But how???

As I said, it just won't let me. How does `replaceWith` wort if it complains when I try using it???

Also, the docs, e.g. https://github.com/jamiebuilds/babel-handbook/blob/master/translations/en/plugin-handbook.md#toc-replacing-a-node and https://github.com/jamiebuilds/babel-docs/blob/master/en_US/authors/guides/transformation-operations.md always show some completely unexplained "`t.`" When I look in other code it's actually some function `t()`, and when I copy that function I need to also copy a bit of other stuff and strange includes like _interop[Something] around a require statement, which I also need to get form somewhere... Cold you please update the docs to explain that "`t.`"??

The examples for replacing nodes in the two docs above don't work! When I try I get an error that the type I'm trying to replace is "expected". WTF??? I want to *replace* it...
How are you using `replaceWith`? You can't replace a node with a comment, you must replace a node with another node. Probably you want to copy the comments to another existing node.

`t()` is because you are looking at the compiled Babel code and it is a reference to `@babel/types`; I suggest reading the original source code in this repository.
@nicolo-ribaudo I know, I read that much. I tried a few things and got an error that a "Declaration" was expected when I tried that in the `TSInterfaceDeclaration` (word from memory, something like that) node handler function (originally consisting only of `path.remove()`).

>  Probably you want to copy the comments to another existing node.

Well, thanks, I'm asking *how*... although I'm not completely clear what exactly is happening. Does the path.remove indeed remove those comments? Or is it a higher level effect?

I found "t()", it was compiled to `_core().types`.
> Well, thanks, I'm asking how... although I'm not completely clear what exactly is happening. Does the path.remove indeed remove those comments?

Yes, because those comments are attached to the node you are removing. You can see it [here](https://astexplorer.net/#/gist/a1f604b8774283ac0dd16dca716dccb3/1b0693e7152218ea57d9f8b52e3c1982a6891240): they are attached to the `ExportNamedDeclaration` node.

You can use `t.inheritsComments(target, origin)` to copy the comments you want to keep to a different node.

Keep in mind that usually removing comments from a removed node is the right behavior, so you probably won't find a generic solution but you need to hard-core what needs to be kept. I'm curious: why do you need JSDoc comments in the compiled output? Developers/dev tools should use the original source code. Doesn't `jsdoc3` support TypeScript code?

> I found "t()", it was compiled to _core().types.

Yeah, which is `@babel/types` re-exported by `@babel/core`.
@lll000111 I ran into this exact issue today and I got around it by adding a semicolon to the end the interface.

For some reason `@babel/preset-typescript` respects semi-colons and will include the file if a semi-colon is present.

Here's quick example:

**src/interface.ts**

```js
/**
 * @global
 * @typedef {Object} FileCreation
 * @property {SHA256Hash} hash - The SHA-256 hash of the contents of a versioned ONE object
 * @property {FileCreationStatus} status - A string constant showing whether the file
 * already existed or if it had to be created.
 */
export interface FileCreation {
    hash: SHA256Hash;
    status: FileCreationStatus;
};
```

**Run babel**
`babel --extensions ".ts" src/ --out-dir dist/`

**dist/interface.js**

```js
/**
 * @global
 * @typedef {Object} FileCreation
 * @property {SHA256Hash} hash - The SHA-256 hash of the contents of a versioned ONE object
 * @property {FileCreationStatus} status - A string constant showing whether the file
 * already existed or if it had to be created.
 */
;
```

Also this is `babelrc` file that I used

**.babelrc**

```js
{
  "presets": [
    ["@babel/preset-env", { "targets": { "node": true } }],
    "@babel/preset-typescript"
  ],
  "plugins": [
    "@babel/plugin-proposal-class-properties",
    "@babel/proposal-object-rest-spread"
  ]
}
```

I hope this works for you!!

> I'm curious: why do you need JSDoc comments in the compiled output?

I explain it in the second paragraph: I'm running jsdoc! To generate the HTML API docs! I **need** **exactly** the JSDoc! In that case the code itself actually doesn't matter at all apart from those declarations that jsdoc uses.

> Keep in mind that usually removing comments from a removed node is the right behavior, 

There is a top-level babelrc setting for that!! If that is `false`it is **not** the right behavior to remove comments.
@nicolo-ribaudo I found something really weird, the issue is more complicated an nuanced. The comment removal is not always happening! If it was so simple as you show in the parser, the comment belong to the parsed statement so it's removed, then it would be a reliable outcome, but it isn't. It seems to be related to `import` statements, at least I can work around it by moving them, see last paragraph below.

[Here is a Babel Repl example](https://babeljs.io/repl/#?babili=false&browsers=&build=&builtIns=false&spec=false&loose=false&code_lz=OQVwzgpgBGAuBOBLAxrYBuAUJg9AKj0yjygAEBbAewBMQAbCIvHbRcgB0vligG9l4EAIawIAWQhgwQgOYQAQuAC-UAGbxK5KMAB0OnOUnS5AWgBG4DNnyFiZWAE92EahFV8AYiAB2qRJW8VAAUNckRIACVJSjoANwgAYTMmFggAD05uKEdnKBDNcIgosBj4pIAeABUAPigAXigAClihOhAIAH4ALihKqAAfPNDCgBlEAGsIKuqASnra2MpEaixcAiZ7Jxc3Tx8_AODhyIgAKwhUJJTMdMyeHOh8sOOzi7N6psEhEu9uqCFvBxzOoLJYrazrOyke6udy8ADyZhesBUlXgQmQ40Q3hkj0KCJOVxuXB4WNE8FU6OgqPRmOxuMg-OmfCIUFZ7COEB69KmNSwrNZghKcU5QwKxyFZTM0z5_MESK5HKiSKSWCU4NsJChWxhfHx52RUAAgnQ6ABlCCwWAMaiEjLE7JbI0m82W61Mhq8FptEWVAA0MFgInAPWAqnoqkQJpcwBUg14n2-PR8428lAA7t5_XAg2AQ3L9dGlKsbBstc4dfDEfqVMazRarS5TYHYOBbbcoMgAnAnXXXY3m-B3rwWayPABVEYeACSIxGAFEACIhsN0CNR6jAX0jqAROcAKTnCUqi7zpwLG8wKi-Ha7sGLOF6EQAmju52I4QA1KcAOQA4r0AAk5ygBI4QXYCRh_YD5DnEY4QAdUwTtvG7CQpFkBRBwaARhFENDjEwsBGhmLAgA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=es2015%2Creact%2Cstage-2&prettier=false&targets=&version=7.4.4&externalPlugins=%40babel%2Fplugin-transform-typescript%407.4.4) 

Try either one of
- Removing the last line
- The first line with the `import` statement does it too
- Move the `import` statement below the TS type definitions

With that import line all comments are gone as reported as my original problem. When that line is not there, all the comments are there all of a sudden!

I now moved all the `import` statements AFTER all the type definitions in all of my files. No comments are removed any more. This *could* work as a workaround, although to my mind at least it isn't logical to have imports after exports (since the exports often use imported types) so my sense of order doesn't like it :)

Besides, I don't like the randomness of this, the comment removals, I think there should at least be consistent behavior (to a normal human developer, of course, somewhere deep inside the parser's bowels this is consistent).

<br>

**UPDATE:** I experimented with the plugin code, removing *everything* (that changes the AST) but

```js
      TSInterfaceDeclaration(path) {
        path.remove();
      },

      TSTypeAliasDeclaration(path) {
        path.remove();
      },
```

and I still get this `import` statement location dependent behavior. So it isn't in the plugin but deeper. IMO node removal should not deliver different results based on the location of `import` statements, that makes no sense to me.

<br>

**Something else** I just remembered probably worth mentioning: I recently switched from Flow to TypeScript, mostly just by renaming files *.js => *.ts. I never noticed anything previously, despite having had pretty much the same Babel workflow, only plugin flow-remove-types instead of the TypeScript plugin which also only removes types. This contradicts the previous paragraphs, where I find that cutting down the code in the TS plugin to just two plain removal statements (which surely flow-remove-types has two) makes no difference.

<br>

@pauloptimizely Thanks! I prefer the workaround I just found, see above, better than adding semicolons to my subjective feeling (and my IDE would disapprove about that too).

Patch:
diff --git a/packages/babel-helper-create-class-features-plugin/src/fields.ts b/packages/babel-helper-create-class-features-plugin/src/fields.ts
--- a/packages/babel-helper-create-class-features-plugin/src/fields.ts
+++ b/packages/babel-helper-create-class-features-plugin/src/fields.ts
@@ -1277,6 +1277,12 @@ export function buildFieldsInitNodes(
     pureStaticNodes: pureStaticNodes.filter(Boolean),
     wrapClass(path: NodePath<t.Class>) {
       for (const prop of props) {
+        // Delete leading comments so that they don't get attached as
+        // trailing comments of the previous sibling.
+        // When transforming props, we explicitly attach their leading
+        // comments to the transformed node with `inheritPropComments`
+        // above.
+        prop.node.leadingComments = null;
         prop.remove();
       }
 
diff --git a/packages/babel-traverse/src/path/comments.ts b/packages/babel-traverse/src/path/comments.ts
--- a/packages/babel-traverse/src/path/comments.ts
+++ b/packages/babel-traverse/src/path/comments.ts
@@ -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,


Test patch:
diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-babel-7/output.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-babel-7/output.js
--- a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-babel-7/output.js
+++ b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-babel-7/output.js
@@ -1,7 +1,6 @@
 class C {
-  // Output should not use `_initialiseProps`
-
   constructor(T) {
+    // Output should not use `_initialiseProps`
     babelHelpers.defineProperty(this, "y", 0);
   }
 }
diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose-babel-7/output.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose-babel-7/output.js
--- a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose-babel-7/output.js
+++ b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose-babel-7/output.js
@@ -1,7 +1,6 @@
 class C {
-  // Output should not use `_initialiseProps`
-
   constructor(T) {
+    // Output should not use `_initialiseProps`
     this.y = 0;
   }
 }
diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose/output.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose/output.js
--- a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose/output.js
+++ b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types-loose/output.js
@@ -1,7 +1,6 @@
 class C {
-  // Output should not use `_initialiseProps`
-
   constructor(T) {
+    // Output should not use `_initialiseProps`
     this.x = void 0;
     this.y = 0;
   }
diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types/output.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types/output.js
--- a/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types/output.js
+++ b/packages/babel-plugin-proposal-class-properties/test/fixtures/compile-to-class/constructor-collision-ignores-types/output.js
@@ -1,7 +1,6 @@
 class C {
-  // Output should not use `_initialiseProps`
-
   constructor(T) {
+    // Output should not use `_initialiseProps`
     babelHelpers.defineProperty(this, "x", void 0);
     babelHelpers.defineProperty(this, "y", 0);
   }
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/declared-types/output.mjs b/packages/babel-plugin-transform-typescript/test/fixtures/exports/declared-types/output.mjs
--- a/packages/babel-plugin-transform-typescript/test/fixtures/exports/declared-types/output.mjs
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/declared-types/output.mjs
@@ -1,4 +1,7 @@
 export class C2 {}
+
+// Not-even E
+
 // everything removed
 export { C2 as C3 }; // only C2->C3
 var BB;
@@ -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
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/input.ts b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/input.ts
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/input.ts
@@ -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;
\ No newline at end of file
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/output.mjs b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/output.mjs
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-1/output.mjs
@@ -0,0 +1,15 @@
+/**
+ * @module
+ */
+
+import '../message-bus';
+
+/**
+ * @typedef {Function} PromiseResolveCb
+ */
+
+/**
+ * @typedef {Function} PromiseRejectCb
+ */
+
+var a;
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/input.ts b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/input.ts
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/input.ts
@@ -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;
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/output.mjs b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/output.mjs
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-2/output.mjs
@@ -0,0 +1,13 @@
+/**
+ * @module
+ */
+
+import '../message-bus';
+
+/**
+ * @typedef {Function} PromiseResolveCb
+ */
+
+/**
+ * @typedef {Function} PromiseRejectCb
+ */
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/input.ts b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/input.ts
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/input.ts
@@ -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;
\ No newline at end of file
diff --git a/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/output.mjs b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/output.mjs
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-transform-typescript/test/fixtures/exports/issue-9916-3/output.mjs
@@ -0,0 +1,14 @@
+/**
+ * @module
+ */
+
+/**
+ * @typedef {Function} PromiseResolveCb
+ */
+
+/**
+ * @typedef {Function} PromiseRejectCb
+ */
+
+var a;
+export {};
diff --git a/packages/babel-traverse/test/fixtures/comments/attachment/output.js b/packages/babel-traverse/test/fixtures/comments/attachment/output.js
--- a/packages/babel-traverse/test/fixtures/comments/attachment/output.js
+++ b/packages/babel-traverse/test/fixtures/comments/attachment/output.js
@@ -2,10 +2,10 @@
 {
   hasPrev;
   /* top */
-  /* left */ /* right */
+  /* left */
+  /* right */
   /* bottom */
 }
-
 {
   /* top */
   /* left */ /* right */
@@ -16,7 +16,6 @@
   hasPrev;
   /* top */
   /* left */
-
   /* right */
   /* bottom */
   hasNext;