Skip to content

Commit

Permalink
Preserve comments and blank lines at top-of-file (#82)
Browse files Browse the repository at this point in the history
Fixes #81 

- This also reverts my `alpha.4` behavior-change where top-of-file
comments always got a newline injected below them. Now we neither inject
nor delete 1 blank line below the last top-of-file-comment.
- This reverts snapshot changes (around top-of-file blank-lines) as
compared to our pre-4.0.0-alpha.4 behavior.
- Fixes some cases where we would introduce a newline when combining type and value imports (reference #54)

---------

Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
  • Loading branch information
fbartho and IanVS committed May 16, 2023
1 parent f7f0689 commit aabc923
Show file tree
Hide file tree
Showing 17 changed files with 352 additions and 78 deletions.
14 changes: 7 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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`.
Expand Down
43 changes: 20 additions & 23 deletions src/utils/__tests__/adjust-comments-on-sorted-nodes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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([]);
});
79 changes: 79 additions & 0 deletions src/utils/__tests__/get-comment-registry.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import {
emptyStatement,
stringLiteral,
type CommentBlock,
type ImportDeclaration,
} from '@babel/types';
import { describe, expect, test } from 'vitest';

import {
attachCommentsToOutputNodes,
CommentAssociation,
getCommentRegistryFromImportDeclarations,
testingOnlyExports,
} from '../get-comment-registry';

describe('getCommentRegistryFromImportDeclarations', () => {
test('is empty if provided no comments or no first-import', () => {
expect(
getCommentRegistryFromImportDeclarations({
firstImport: emptyStatement() as any,
outputNodes: [],
}),
).toEqual([]);
});
});

describe('attachCommentsToOutputNodes', () => {
test('throws when missing inputs', () => {
expect(() =>
attachCommentsToOutputNodes([], [], emptyStatement() as any),
).toThrow(
new Error(
"Fatal Internal Error: Can't attach comments to empty output",
),
);
});
test('does not inject an EmptyStatement if there are no top-of-file comments', () => {
const firstImport = {
type: 'ImportDeclaration',
specifiers: [],
source: stringLiteral('foo'),
} as ImportDeclaration;
const outputNodes = [firstImport];

attachCommentsToOutputNodes([], outputNodes, firstImport);

expect(outputNodes[0].type).not.toEqual('EmptyStatement');
});
test("injects an EmptyStatement if there's a top-of-file comment", () => {
const firstImport = {
type: 'ImportDeclaration',
specifiers: [],
source: stringLiteral('foo'),
} as ImportDeclaration;
const comment = {
type: 'CommentBlock',
value: '@prettier',
} as CommentBlock;
const outputNodes = [firstImport];

attachCommentsToOutputNodes(
[
{
needsTopOfFileOwner: true,
comment,
ownerIsSpecifier: false,
commentId: testingOnlyExports.nodeId(comment),
owner: firstImport,
association: CommentAssociation.trailing,
processingPriority: 0,
},
],
outputNodes,
firstImport,
);

expect(outputNodes[0].type).toEqual('EmptyStatement');
});
});
22 changes: 21 additions & 1 deletion src/utils/__tests__/get-sorted-import-specifiers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ test('should return correct sorted nodes with default import', () => {
]);
});

test('should group type imports after value imports', () => {
test('should group type imports after value imports - typescript', () => {
const code = `import Component, { type TypeB, filter, type TypeA, reduce, eventHandler } from '@server/z';`;
const [importNode] = getImportNodes(code, {
plugins: ['typescript'],
Expand All @@ -50,3 +50,23 @@ test('should group type imports after value imports', () => {
'TypeB',
]);
});

test('should group type imports after value imports - flow', () => {
const code = `import Component, { type TypeB, filter, type TypeA, reduce, eventHandler } from '@server/z';`;
const [importNode] = getImportNodes(code, {
plugins: ['flow'],
});
const sortedImportSpecifiers = getSortedImportSpecifiers(importNode);
const specifiersList = getSortedNodesModulesNames(
sortedImportSpecifiers.specifiers,
);

expect(specifiersList).toEqual([
'Component',
'eventHandler',
'filter',
'reduce',
'TypeA',
'TypeB',
]);
});
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ import { Junk } from "junk-group-1";
import "./side-effects1";
// C, E and D will be separated from A, B because side-effects in-between
import { D, type C, type E } from "a";
// prettier-ignore
Expand Down
12 changes: 7 additions & 5 deletions src/utils/adjust-comments-on-sorted-nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,22 @@ import {
* @returns A copied and adjusted set of nodes, containing comments
*/
export const adjustCommentsOnSortedNodes = (
originalDeclarationNodes: ImportDeclaration[],
finalNodes: ImportOrLine[],
originalDeclarationNodes: readonly ImportDeclaration[],
finalNodes: readonly ImportOrLine[],
) => {
const outputNodes: ImportDeclaration[] = finalNodes.filter(
(n) => n.type === 'ImportDeclaration',
) as ImportDeclaration[];
if (originalDeclarationNodes.length === 0 || outputNodes.length === 0) {
// Nothing to do, because there are no ImportDeclarations!
return finalNodes;
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
Expand All @@ -44,7 +46,7 @@ export const adjustCommentsOnSortedNodes = (
return noDirectCommentsNode;
});

attachCommentsToOutputNodes(registry, finalNodesClone);
attachCommentsToOutputNodes(registry, finalNodesClone, firstImport);

return finalNodesClone;
};
2 changes: 1 addition & 1 deletion src/utils/get-all-comments-from-nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type {
import { SomeSpecifier } from '../types';

export const getAllCommentsFromNodes = (
nodes: (Directive | Statement | SomeSpecifier)[],
nodes: readonly (Directive | Statement | SomeSpecifier)[],
) =>
nodes.reduce((acc, node) => {
if (
Expand Down
Loading

0 comments on commit aabc923

Please sign in to comment.