Behavior Change: All comments before firstImport are treated as top-of-file

README.md

@@ -12,12 +12,12 @@ Since then more critical features & fixes have been added, and the options have
 
 **Features not currently supported by upstream:**
 
--   Do not re-order across side-effect imports
--   Combine imports from the same source
--   Combine type and value imports
--   Type import grouping with `<TYPES>` keyword
--   Sorts node.js builtin modules to top
--   Custom import order separation
+- Do not re-order across side-effect imports
+- Combine imports from the same source
+- Combine type and value imports
+- Type import grouping with `<TYPES>` keyword
+- Sorts node.js builtin modules to top
+- Custom import order separation
 
 [We welcome contributions!](./CONTRIBUTING.md)
 
@@ -276,7 +276,7 @@ entire import statements can be ignored, line comments (`// prettier-ignore`) ar
 
 We make the following attempts at keeping comments in your imports clean:
 
-- If you have one or more comments at the top of the file, we will keep them at the top as long as there is a blank line before your first import statement.
+- If you have one or more comments at the top of the file, we will keep them at the top.
 - Comments on lines after the final import statement will not be moved. (Runtime-code between imports will be moved below all the imports).
 - In general, if you place a comment on the same line as an Import `Declaration` or `*Specifier`, we will keep it attached to that same specifier if it moves around.
 - Other comments are preserved, and are generally considered "leading" comments for the subsequent Import `Declaration` or `*Specifier`.

src/utils/__tests__/adjust-comments-on-sorted-nodes.spec.ts

@@ -23,18 +23,14 @@ test('it preserves the single leading comment for each import declaration', () =
     expect(importNodes).toHaveLength(3);
     const finalNodes = [importNodes[2], importNodes[1], importNodes[0]];
     const adjustedNodes = adjustCommentsOnSortedNodes(importNodes, finalNodes);
-    expect(adjustedNodes).toHaveLength(4);
-    // First node is a dummy EmptyStatement
-    expect(adjustedNodes[0].type).toEqual('EmptyStatement');
-    expect(leadingComments(adjustedNodes[0])).toEqual([]);
+    expect(adjustedNodes).toHaveLength(3);
+    expect(leadingComments(adjustedNodes[0])).toEqual([' comment a']);
     expect(trailingComments(adjustedNodes[0])).toEqual([]);
-    expect(leadingComments(adjustedNodes[1])).toEqual([' comment a']);
+    expect(leadingComments(adjustedNodes[1])).toEqual([' comment b']);
     expect(trailingComments(adjustedNodes[1])).toEqual([]);
-    expect(leadingComments(adjustedNodes[2])).toEqual([' comment b']);
-    expect(trailingComments(adjustedNodes[2])).toEqual([]);
     // Import from "c" has no leading comment, and the trailing was kept with "b"
-    expect(leadingComments(adjustedNodes[3])).toEqual([]);
-    expect(trailingComments(adjustedNodes[3])).toEqual([]);
+    expect(leadingComments(adjustedNodes[2])).toEqual([]);
+    expect(trailingComments(adjustedNodes[2])).toEqual([]);
 });
 
 test('it preserves multiple leading comments for each import declaration', () => {
@@ -52,21 +48,21 @@ test('it preserves multiple leading comments for each import declaration', () =>
     expect(importNodes).toHaveLength(3);
     const finalNodes = [importNodes[2], importNodes[1], importNodes[0]];
     const adjustedNodes = adjustCommentsOnSortedNodes(importNodes, finalNodes);
-    expect(adjustedNodes).toHaveLength(4);
-    expect(leadingComments(adjustedNodes[1])).toEqual([
+    expect(adjustedNodes).toHaveLength(3);
+    expect(leadingComments(adjustedNodes[0])).toEqual([
         ' comment a1',
         ' comment a2',
         ' comment a3',
     ]);
-    expect(trailingComments(adjustedNodes[1])).toEqual([]);
-    expect(leadingComments(adjustedNodes[2])).toEqual([
+    expect(trailingComments(adjustedNodes[0])).toEqual([]);
+    expect(leadingComments(adjustedNodes[1])).toEqual([
         ' comment b1',
         ' comment b2',
         ' comment b3',
     ]);
+    expect(trailingComments(adjustedNodes[1])).toEqual([]);
+    expect(leadingComments(adjustedNodes[2])).toEqual([]);
     expect(trailingComments(adjustedNodes[2])).toEqual([]);
-    expect(leadingComments(adjustedNodes[3])).toEqual([]);
-    expect(trailingComments(adjustedNodes[3])).toEqual([]);
 });
 
 test('it does not move comments more than one line before all import declarations', () => {
@@ -81,15 +77,16 @@ test('it does not move comments more than one line before all import declaration
     const finalNodes = [importNodes[2], importNodes[1], importNodes[0]];
     const adjustedNodes = adjustCommentsOnSortedNodes(importNodes, finalNodes);
     expect(adjustedNodes).toHaveLength(4);
-    // Comment c1 is more than one line above the first import, so it stays with the top-of-file
-    expect(leadingComments(adjustedNodes[0])).toEqual([' comment c1']);
+    // Comment c1 is above the first import, so it stays with the top-of-file attached to a dummy statement
+    expect(adjustedNodes[0].type).toEqual('EmptyStatement');
+    expect(trailingComments(adjustedNodes[0])).toEqual([
+        ' comment c1',
+        ' comment c2',
+    ]);
 
     expect(leadingComments(adjustedNodes[2])).toEqual([]);
     expect(trailingComments(adjustedNodes[2])).toEqual([]);
     expect(trailingComments(adjustedNodes[3])).toEqual([]);
-
-    // Comment c2 is attached to import from "c"
-    expect(leadingComments(adjustedNodes[3])).toEqual([' comment c2']);
 });
 
 test('it does not affect comments after all import declarations', () => {
@@ -103,11 +100,11 @@ test('it does not affect comments after all import declarations', () => {
     expect(importNodes).toHaveLength(3);
     const finalNodes = [importNodes[2], importNodes[1], importNodes[0]];
     const adjustedNodes = adjustCommentsOnSortedNodes(importNodes, finalNodes);
-    expect(adjustedNodes).toHaveLength(4);
+    expect(adjustedNodes).toHaveLength(3);
+    expect(leadingComments(adjustedNodes[0])).toEqual([]);
+    expect(trailingComments(adjustedNodes[0])).toEqual([]);
     expect(leadingComments(adjustedNodes[1])).toEqual([]);
     expect(trailingComments(adjustedNodes[1])).toEqual([]);
     expect(leadingComments(adjustedNodes[2])).toEqual([]);
     expect(trailingComments(adjustedNodes[2])).toEqual([]);
-    expect(leadingComments(adjustedNodes[3])).toEqual([]);
-    expect(trailingComments(adjustedNodes[3])).toEqual([]);
 });

src/utils/adjust-comments-on-sorted-nodes.ts

@@ -26,9 +26,11 @@ export const adjustCommentsOnSortedNodes = (
         return finalNodes;
     }
 
+    const firstImport = originalDeclarationNodes[0];
+
     const registry = getCommentRegistryFromImportDeclarations({
         outputNodes,
-        firstImport: originalDeclarationNodes[0],
+        firstImport,
     });
 
     // Make a copy of the nodes for easier debugging & remove the existing comments to reattach them
@@ -44,7 +46,7 @@ export const adjustCommentsOnSortedNodes = (
         return noDirectCommentsNode;
     });
 
-    attachCommentsToOutputNodes(registry, finalNodesClone);
+    attachCommentsToOutputNodes(registry, finalNodesClone, firstImport);
 
     return finalNodesClone;
 };

src/utils/get-comment-registry.ts

@@ -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;
@@ -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
@@ -147,22 +151,33 @@ 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') {
             const currentOwnerIsFirstImport =
                 nodeId(owner) === nodeId(firstImport);
 
-            // endsMoreThanOneLineAboveOwner is used with firstImport to protect top-of-file comments,
-            // and pick the right ImportSpecifier when Specifiers are re-sorted
-            const endsMoreThanOneLineAboveOwner =
-                (comment.loc?.end.line || 0) < (owner.loc?.start.line || 0) - 1;
+            const endsBeforeOwner =
+                (comment.loc?.end.line || 0) <= (owner.loc?.start.line || 0);
 
-            if (currentOwnerIsFirstImport && endsMoreThanOneLineAboveOwner) {
+            if (currentOwnerIsFirstImport && endsBeforeOwner) {
                 debugLog?.('Found a disconnected leading comment', {
                     comment,
                     owner,
@@ -350,58 +365,156 @@ export const getCommentRegistryFromImportDeclarations = ({
 export function attachCommentsToOutputNodes(
     commentEntriesFromRegistry: CommentEntry[],
     outputNodes: ImportOrLine[],
+    /** Original declaration, not the re-sorted output-node! */
+    firstImport: ImportDeclaration,
 ) {
     if (outputNodes.length === 0) {
         // attachCommentsToOutputNodes implies that there's at least one output node so this shouldn't happen
         throw new Error(
             "Fatal Internal Error: Can't attach comments to empty output",
         );
     }
-    if (outputNodes[0].type !== 'EmptyStatement') {
-        // Put in a dummy empty statement to attach top-of-file-comments to if one was not provided
-        outputNodes.unshift(emptyStatement());
+    if (firstImport == null) {
+        throw new Error(
+            "Fatal Internal Error: Can't attach comments if there was no firstImport",
+        );
     }
 
+    /** 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);
             }
         }
     }
 
+    const newFirstImport = outputNodes[0];
+
     for (const commentEntry of commentEntriesFromRegistry) {
-        const { owner, comment, association, needsTopOfFileOwner } =
-            commentEntry;
+        const {
+            owner,
+            comment,
+            association,
+            needsTopOfFileOwner,
+            needsLastSpecifierOwner,
+        } = commentEntry;
+
+        if (needsTopOfFileOwner && outputNodes[0].type !== 'EmptyStatement') {
+            // Put in a dummy empty statement to attach top-of-file-comments to if one was not provided
+            const dummy = emptyStatement();
+            dummy.loc = {
+                start: { line: 0, column: 0 },
+                end: { line: 0, column: 0 },
+            };
+            outputNodes.unshift(dummy);
+
+            // Put the first import in the right spot, where the original first import started
+            // Otherwise, comments at the top of the file will not be formatted correctly.
+            // This is a little tricky, because the new first import might have leading comments,
+            // and we have to move the node and all comments the same distance
+            const commentHeight = getHeightOfLeadingComments(newFirstImport);
+            const originalLoc = newFirstImport.loc;
+            if (firstImport.loc && originalLoc) {
+                newFirstImport.loc = {
+                    start: {
+                        ...firstImport.loc?.start,
+                        line: firstImport.loc?.start.line + commentHeight,
+                    },
+                    end: {
+                        ...firstImport.loc?.end,
+                        line: firstImport.loc?.end.line + commentHeight,
+                    },
+                };
+                const moveDist =
+                    originalLoc.start.line - newFirstImport.loc.start.line;
+
+                for (const commentType of orderedCommentKeysToRegister) {
+                    newFirstImport[commentType]?.forEach((c) => {
+                        if (c.loc) {
+                            c.loc.start.line -= moveDist;
+                            c.loc.end.line -= moveDist;
+                        }
+                    });
+                }
+            }
+        }
 
-        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");
         }
 
+        // // Since we mucked with the loc of the newFirstImport, we need to be careful to
+        // // keep its comments in the right place, so adjust their loc too
+        if (
+            ownerNode === newFirstImport &&
+            association !== CommentAssociation.leading &&
+            comment.loc &&
+            ownerNode.loc &&
+            !needsLastSpecifierOwner
+        ) {
+            comment.loc.start.line = ownerNode.loc.start.line;
+        }
+
         // addComments(ownerNode, association, [comment]);
         // using Babel's addComments will reverse the comments if you iteratively attach them, so push them directly
-        const commentCollection = (ownerNode[
-            CommentAssociationByValue[association]
-        ] = ownerNode[CommentAssociationByValue[association]] || []);
+        const commentType = needsTopOfFileOwner
+            ? 'trailingComments' // use trailing comments to preserve trailing blank line if present
+            : CommentAssociationByValue[association];
+        const commentCollection = (ownerNode[commentType] =
+            ownerNode[commentType] || []);
         (commentCollection as Comment[]).push(comment);
     }
+}
 
-    if (Array.isArray(outputNodes[0].leadingComments)) {
-        if (outputNodes[0].leadingComments.length > 0) {
-            // Convert this to a newline node!
-            outputNodes[0] = {
-                ...newLineNode, // Inject a newline after top-of-file comments
-                leadingComments: outputNodes[0].leadingComments,
-            };
-        } else {
-            outputNodes.shift(); // Remove the empty statement
-        }
+function getHeightOfLeadingComments(node: ImportOrLine) {
+    if (
+        Array.isArray(node.leadingComments) &&
+        node.leadingComments.length &&
+        node.leadingComments[0].loc &&
+        node.loc
+    ) {
+        return node.loc.start.line - node.leadingComments[0].loc.start.line;
     }
+    return 0;
 }

src/utils/get-sorted-nodes-by-import-order.ts

@@ -27,7 +27,7 @@ export const getSortedNodesByImportOrder: GetSortedNodes = (
         importOrder = [THIRD_PARTY_MODULES_SPECIAL_WORD, ...importOrder];
     }
 
-    // IDEA: We could make built-ins a special word, if people do not want them up top
+    // Opinionated decision: builtin modules should always be first
     importOrder = [BUILTIN_MODULES, ...importOrder];
 
     const importOrderGroups = importOrder.reduce<ImportGroups>(
@@ -42,6 +42,7 @@ export const getSortedNodesByImportOrder: GetSortedNodes = (
         {},
     );
 
+    // Select just the SPECIAL WORDS and the matchers
     const sanitizedImportOrder = importOrder.filter(
         (group) =>
             !isCustomGroupSeparator(group) &&