Skip to content

Commit

Permalink
Preserve trailing comments on specifiers (#80)
Browse files Browse the repository at this point in the history
Fixes #79

Trailing comments on specifiers don't get automatically saved like
top-level ones do. This ensures that such comments are not lost


Before:
```ts
import {
    b2,a2,
    // @ts-expect-error
} from "b";

import {
    b1,
    a1,

    // @ts-expect-error
} from "a";
```
After
```ts
import {
    a1,
    b1,
    // @ts-expect-error
} from "a";
import {
    a2,
    b2,
    // @ts-expect-error
} from "b";
```

---------

Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
  • Loading branch information
fbartho and IanVS committed May 15, 2023
1 parent dcff9c2 commit f7f0689
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 8 deletions.
72 changes: 64 additions & 8 deletions src/utils/get-comment-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ const orderedCommentKeysToRegister = [
export interface CommentEntry {
owner: ImportDeclaration | SomeSpecifier;
ownerIsSpecifier: boolean;
// Special case for leaving comments at top-of-file
/** Special case for leaving comments at top-of-file */
needsTopOfFileOwner?: boolean;
/** Comments that follow the last specifier must stay at the bottom of their import block! */
needsLastSpecifierOwner?: boolean;

commentId: string;
comment: Comment;
Expand All @@ -69,6 +71,8 @@ const MAX_COUNT_OF_LIKELY_IMPORT_STATEMENTS = 10000;
enum DeferredCommentClaimPriorityAdjustment {
leadingSpecifier = MAX_COUNT_OF_LIKELY_IMPORT_STATEMENTS * 1,
leadingAboveAllImports = MAX_COUNT_OF_LIKELY_IMPORT_STATEMENTS * 2,
/** This must stay a trailing comment, because it might be a directive preceding `} from "./foo"` */
trailingCommentForSpecifier = MAX_COUNT_OF_LIKELY_IMPORT_STATEMENTS * 3,
}

const debugLog: typeof console.debug | undefined = undefined as any; // undefined as any, because typescript is too smart
Expand Down Expand Up @@ -147,10 +151,23 @@ const attachCommentsToRegistryMap = ({
if (isSameLineAsCurrentOwner) {
commentRegistry.set(commentId, commentEntry);
} else {
// This comment is either a leading comment on the next node, or it's an unrelated comment following the imports
// Trailing comment, not on the same line, so either it will get attached correctly, or it will be dropped below imports
// -- will automatically be attached from other nodes, or will fall to bottom of imports
// [Intentional empty block]
// This comment is actually either a leading comment on the next node,
// or it's an unrelated comment following the imports
// or it's a trailing comment on the last specifier inside a declaration

if (ownerIsSpecifier) {
// Specifier comments will just vanish if not present on an output node.
deferredCommentClaims.push({
...commentEntry,
needsLastSpecifierOwner: true,
processingPriority:
commentEntry.processingPriority +
DeferredCommentClaimPriorityAdjustment.trailingCommentForSpecifier,
});
} else {
// [Intentional empty block] - top-level comments will be attached as a leading attachment,
// on another node or will be preserved automatically by babel & fall to bottom of imports
}
}
continue; // Unnecessary, but explicit
} else if (attachmentKey === 'leadingComments') {
Expand Down Expand Up @@ -362,24 +379,63 @@ export function attachCommentsToOutputNodes(
outputNodes.unshift(emptyStatement());
}

/** Store a mapping of Specifier to ImportDeclaration */
const parentNodeId = (specifier: SomeSpecifier) =>
`parent::${nodeId(specifier)}`;

const outputRegistry = new Map<string, ImportRelated>();
for (const outputNode of outputNodes) {
outputRegistry.set(nodeId(outputNode), outputNode);
if (outputNode.type === 'ImportDeclaration') {
for (const specifier of outputNode.specifiers) {
outputRegistry.set(nodeId(specifier), specifier);
outputRegistry.set(parentNodeId(specifier), outputNode);
}
}
}

for (const commentEntry of commentEntriesFromRegistry) {
const { owner, comment, association, needsTopOfFileOwner } =
commentEntry;
const {
owner,
comment,
association,
needsTopOfFileOwner,
needsLastSpecifierOwner,
} = commentEntry;

const ownerNode = needsTopOfFileOwner
let ownerNode = needsTopOfFileOwner
? outputNodes[0]
: outputRegistry.get(nodeId(owner));

if (needsLastSpecifierOwner) {
const parentDeclaration = outputRegistry.get(
parentNodeId(owner as SomeSpecifier),
) as ImportDeclaration | undefined;

if (
!parentDeclaration ||
(parentDeclaration.specifiers?.length || 0) === 0
) {
throw new Error(
"Fatal Internal Error: Couldn't find parent declaration for a specifier",
);
}
const lastSpecifier =
parentDeclaration.specifiers[
parentDeclaration.specifiers.length - 1
];

ownerNode = lastSpecifier;

// Start the comment on the line below the owner, to avoid gaps
if (
comment.loc?.start.line !== undefined &&
ownerNode.loc?.end.line
) {
comment.loc.start.line = ownerNode.loc?.end.line + 1;
}
}

if (!ownerNode) {
// Shouldn't be possible if you called this helper with the right inputs!
throw new Error("Fatal Internal Error: Couldn't find owner node");
Expand Down
44 changes: 44 additions & 0 deletions tests/ImportCommentsPreserved/__snapshots__/ppsi.spec.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,47 @@ import {
// Rest of file below here
`;

exports[`import-comments-trailing-specifier-comment-lost.ts - typescript-verify > import-comments-trailing-specifier-comment-lost.ts 1`] = `
import {
b3,
a3,
/* @ts-expect-error*/
} from "c";
import {
b4,
a4,
// @ts-expect-error
} from "c2";
import {
b2,a2,
// @ts-expect-error
} from "b";
import {
b1,
a1,
// @ts-expect-error
} from "a";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
import {
a1,
b1,
// @ts-expect-error
} from "a";
import {
a2,
b2,
// @ts-expect-error
} from "b";
import { a3, b3 } from /* @ts-expect-error*/ "c";
import {
a4,
b4,
// @ts-expect-error
} from "c2";
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import {
b3,
a3,
/* @ts-expect-error*/
} from "c";

import {
b4,
a4,
// @ts-expect-error
} from "c2";

import {
b2,a2,
// @ts-expect-error
} from "b";

import {
b1,
a1,

// @ts-expect-error
} from "a";

0 comments on commit f7f0689

Please sign in to comment.