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.

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

Fix behavior around comments embedded in imports #71

Merged
merged 25 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4b0fd09
Add irritating test cases for comments around imports
fbartho May 11, 2023
919b5da
Change test-cases so top-of-file comments have a gap (otherwise they …
fbartho May 11, 2023
0a96f12
Initial types & constants for ImportComments Preservation
fbartho May 11, 2023
c516cf2
Fix get-all-comments
fbartho May 11, 2023
b69eb21
Change test-cases so top-of-file comments have a gap (more tests)
fbartho May 11, 2023
bda233c
Feature: Update Import-adjacent Comment Adjustment to handle all the …
fbartho May 11, 2023
0efd6c3
Remove a no-longer-needed dependency lodash.isequal
fbartho May 11, 2023
18c3045
Formatting tweaks
fbartho May 11, 2023
cec6f65
Minor test cleanup
fbartho May 11, 2023
743e16a
lol forgot to format
fbartho May 11, 2023
2f300ef
Merge branch 'next' into fb/fix-import-comments
fbartho May 11, 2023
8fcda28
Update for new Babel, and remove workarounds
fbartho May 11, 2023
b6965f8
Finish patch cleanup
fbartho May 11, 2023
6eadf55
Unnecessary let
fbartho May 11, 2023
ec11257
Use `node.loc.end.line` instead of `start.line` in certain comparison…
fbartho May 12, 2023
fca0377
PR Feedback: only-same-line-comments should be treated as trailing
fbartho May 12, 2023
90bb398
Merge branch 'next' into fb/fix-import-comments
fbartho May 12, 2023
2d54d8e
Seeing CI-flaking? Disable the yarn-cache in-case that's it
fbartho May 12, 2023
8b22a8e
Revert "Seeing CI-flaking? Disable the yarn-cache in-case that's it"
fbartho May 12, 2023
aae640e
Accidentally stashed this critical line!
fbartho May 12, 2023
c68ca80
Small test tweaks
IanVS May 12, 2023
58c8d01
Rename id -> commentId for clarity
IanVS May 12, 2023
375be16
Clean up lastImport logic
IanVS May 12, 2023
cbf6839
Add test for multi-line comment block spanning lines
IanVS May 12, 2023
7bd1f5e
Simplify readme just a tad
IanVS May 12, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Since then more critical features & fixes have been added, and the options have
- [`importOrderTypeScriptVersion`](#importordertypescriptversion)
- [`importOrderParserPlugins`](#importorderparserplugins)
- [Prevent imports from being sorted](#prevent-imports-from-being-sorted)
- [Comments](#comments)
- [FAQ / Troubleshooting](#faq--troubleshooting)
- [Compatibility](#compatibility)
- [Contribution](#contribution)
Expand Down Expand Up @@ -271,6 +272,15 @@ This will keep the `zealand` import at the top instead of moving it below the `a
entire import statements can be ignored, line comments (`// prettier-ignore`) are recommended over inline comments
(`/* prettier-ignore */`).

### Comments

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.
- 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`.

## FAQ / Troubleshooting

Having some trouble or an issue? You can check [FAQ / Troubleshooting section](./docs/TROUBLESHOOTING.md).
Expand Down
2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,12 @@
"@babel/traverse": "^7.21.5",
"@babel/types": "^7.21.5",
"lodash.clone": "^4.5.0",
"lodash.isequal": "^4.5.0",
"semver": "^7.5.0"
},
"devDependencies": {
"@types/babel__generator": "^7.6.4",
"@types/babel__traverse": "^7.18.3",
"@types/lodash.clone": "4.5.7",
"@types/lodash.isequal": "4.5.6",
"@types/node": "^18.15.13",
"@types/prettier": "^2.7.2",
"@types/semver": "^7.3.13",
Expand Down
12 changes: 12 additions & 0 deletions src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,18 @@ export const TYPES_SPECIAL_WORD = '<TYPES>';
const PRETTIER_PLUGIN_SORT_IMPORTS_NEW_LINE =
'PRETTIER_PLUGIN_SORT_IMPORTS_NEW_LINE';

/** Use this to force a newline at top-level scope (good for newlines generated between import blocks) */
export const newLineNode = expressionStatement(
stringLiteral(PRETTIER_PLUGIN_SORT_IMPORTS_NEW_LINE),
);
/** Use this if you want to force a newline, but you're attaching to leading/inner/trailing Comments */
export const forceANewlineUsingACommentStatement = () => ({
type: 'CommentLine' as const,
value: 'PRETTIER_PLUGIN_SORT_IMPORTS_NEWLINE_COMMENT',
start: -1,
end: -1,
loc: { start: { line: -1, column: -1 }, end: { line: -1, column: -1 } },
});

export const injectNewlinesRegex =
/("PRETTIER_PLUGIN_SORT_IMPORTS_NEW_LINE";|\/\/PRETTIER_PLUGIN_SORT_IMPORTS_NEWLINE_COMMENT)/gi;
1 change: 1 addition & 0 deletions src/preprocessors/preprocessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export function preprocessor(code: string, options: PrettierOptions): string {
const allOriginalImportNodes: ImportDeclaration[] = [];
const parserOptions: ParserOptions = {
sourceType: 'module',
attachComment: true,
plugins: getExperimentalParserPlugins(importOrderParserPlugins),
};

Expand Down
20 changes: 18 additions & 2 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import { ExpressionStatement, ImportDeclaration } from '@babel/types';
import {
type EmptyStatement,
type ExpressionStatement,
type ImportDeclaration,
type ImportDefaultSpecifier,
type ImportNamespaceSpecifier,
type ImportSpecifier,
} from '@babel/types';
import { RequiredOptions } from 'prettier';

import { PluginConfig } from '../types';
Expand Down Expand Up @@ -28,7 +35,16 @@ export interface ImportChunk {
}

export type ImportGroups = Record<string, ImportDeclaration[]>;
export type ImportOrLine = ImportDeclaration | ExpressionStatement;
export type ImportOrLine =
| ImportDeclaration
| ExpressionStatement
| EmptyStatement;

export type SomeSpecifier =
| ImportSpecifier
| ImportDefaultSpecifier
| ImportNamespaceSpecifier;
export type ImportRelated = ImportOrLine | SomeSpecifier;

export type GetSortedNodes = (
nodes: ImportDeclaration[],
Expand Down
49 changes: 27 additions & 22 deletions src/utils/__tests__/adjust-comments-on-sorted-nodes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,18 @@ 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(3);
expect(leadingComments(adjustedNodes[0])).toEqual([' comment a']);
expect(adjustedNodes).toHaveLength(4);
// First node is a dummy EmptyStatement
expect(adjustedNodes[0].type).toEqual('EmptyStatement');
expect(leadingComments(adjustedNodes[0])).toEqual([]);
expect(trailingComments(adjustedNodes[0])).toEqual([]);
expect(leadingComments(adjustedNodes[1])).toEqual([' comment b']);
expect(leadingComments(adjustedNodes[1])).toEqual([' comment a']);
expect(trailingComments(adjustedNodes[1])).toEqual([]);
expect(leadingComments(adjustedNodes[2])).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([]);
});

test('it preserves multiple leading comments for each import declaration', () => {
Expand All @@ -47,24 +52,24 @@ 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(3);
expect(leadingComments(adjustedNodes[0])).toEqual([
expect(adjustedNodes).toHaveLength(4);
expect(leadingComments(adjustedNodes[1])).toEqual([
' comment a1',
' comment a2',
' comment a3',
]);
expect(trailingComments(adjustedNodes[0])).toEqual([]);
expect(leadingComments(adjustedNodes[1])).toEqual([
expect(trailingComments(adjustedNodes[1])).toEqual([]);
expect(leadingComments(adjustedNodes[2])).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 at before all import declarations', () => {
test('it does not move comments more than one line before all import declarations', () => {
const importNodes = getImportNodes(`
// comment c1
// comment c2
Expand All @@ -75,16 +80,16 @@ test('it does not move comments at before all import declarations', () => {
expect(importNodes).toHaveLength(3);
const finalNodes = [importNodes[2], importNodes[1], importNodes[0]];
const adjustedNodes = adjustCommentsOnSortedNodes(importNodes, finalNodes);
expect(adjustedNodes).toHaveLength(3);
expect(leadingComments(adjustedNodes[0])).toEqual([
' comment c1',
' comment c2',
]);
expect(trailingComments(adjustedNodes[0])).toEqual([]);
expect(leadingComments(adjustedNodes[1])).toEqual([]);
expect(trailingComments(adjustedNodes[1])).toEqual([]);
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']);

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 @@ -98,11 +103,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(3);
expect(leadingComments(adjustedNodes[0])).toEqual([]);
expect(trailingComments(adjustedNodes[0])).toEqual([]);
expect(adjustedNodes).toHaveLength(4);
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([]);
});
18 changes: 4 additions & 14 deletions src/utils/__tests__/get-code-from-ast.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import { getImportNodes } from '../get-import-nodes';
import { getSortedNodes } from '../get-sorted-nodes';

test('sorts imports correctly', () => {
const code = `// first comment
// second comment
import z from 'z';
const code = `import z from 'z';
import c from 'c';
import g from 'g';
import t from 't';
Expand All @@ -26,10 +24,7 @@ import a from 'a';
directives: [],
});
expect(format(formatted, { parser: 'babel' })).toEqual(
`// first comment
// second comment

import a from "a";
`import a from "a";
fbartho marked this conversation as resolved.
Show resolved Hide resolved
import c from "c";
import g from "g";
import k from "k";
Expand All @@ -40,9 +35,7 @@ import z from "z";
});

test('merges duplicate imports correctly', () => {
const code = `// first comment
// second comment
import z from 'z';
const code = `import z from 'z';
import c from 'c';
import g from 'g';
import t from 't';
Expand All @@ -64,10 +57,7 @@ import type {See} from 'c';
directives: [],
});
expect(format(formatted, { parser: 'babel' })).toEqual(
`// first comment
// second comment

import a, { b, type Bee } from "a";
`import a, { b, type Bee } from "a";
import c, { type C, type See } from "c";
import g from "g";
import k from "k";
Expand Down
18 changes: 9 additions & 9 deletions src/utils/__tests__/get-sorted-nodes.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import { getSortedNodes } from '../get-sorted-nodes';
import { getSortedNodesModulesNames } from '../get-sorted-nodes-modules-names';
import { getSortedNodesNamesAndNewlines } from '../get-sorted-nodes-names-and-newlines';

const code = `// first comment
// second comment
const code = `
import "se3";
import z from 'z';
import c, { cD } from 'c';
Expand Down Expand Up @@ -51,13 +50,14 @@ test('it returns all sorted nodes, preserving the order side effect nodes', () =
'se2',
'',
]);
expect(
sorted
.filter((node) => node.type === 'ImportDeclaration')
.map((importDeclaration) =>
getSortedNodesModulesNames(importDeclaration.specifiers),
),
).toEqual([

const result2 = sorted
.filter((node) => node.type === 'ImportDeclaration')
.map((importDeclaration) =>
getSortedNodesModulesNames(importDeclaration.specifiers),
);

expect(result2).toEqual([
[],
['c', 'cD'],
['g'],
Expand Down
53 changes: 30 additions & 23 deletions src/utils/adjust-comments-on-sorted-nodes.ts
Original file line number Diff line number Diff line change
@@ -1,43 +1,50 @@
import {
addComments,
removeComments,
type ImportDeclaration,
} from '@babel/types';
import clone from 'lodash.clone';
import isEqual from 'lodash.isequal';
import { removeComments, type ImportDeclaration } from '@babel/types';

import { ImportOrLine } from '../types';
import {
attachCommentsToOutputNodes,
getCommentRegistryFromImportDeclarations,
} from './get-comment-registry';

/**
* Takes the original nodes before sorting and the final nodes after sorting.
* Adjusts the comments on the final nodes so that they match the comments as
* they were in the original nodes.
* @param nodes A list of nodes in the order as they were originally.
* @param originalDeclarationNodes A list of nodes in the order as they were originally.
* @param finalNodes The same set of nodes, but in the final sorting order.
* @returns A copied and adjusted set of nodes, containing comments
*/
export const adjustCommentsOnSortedNodes = (
nodes: ImportDeclaration[],
originalDeclarationNodes: ImportDeclaration[],
finalNodes: ImportOrLine[],
) => {
// We will mutate a copy of the finalNodes, and extract comments from the original
const finalNodesClone = finalNodes.map(clone);

const firstNodesComments = nodes[0].leadingComments;

// Remove all comments from sorted nodes
finalNodesClone.forEach(removeComments);
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;
}

// insert comments other than the first comments
finalNodesClone.forEach((node, index) => {
if (isEqual(nodes[0].loc, node.loc)) return;
const registry = getCommentRegistryFromImportDeclarations({
outputNodes,
firstImport: originalDeclarationNodes[0],
});

addComments(node, 'leading', finalNodes[index].leadingComments || []);
// Make a copy of the nodes for easier debugging & remove the existing comments to reattach them
// (removeComments clones the nodes internally, so we don't need to do that ourselves)
const finalNodesClone = finalNodes.map((n) => {
const noDirectCommentsNode = removeComments(n);
if (noDirectCommentsNode.type === 'ImportDeclaration') {
// Remove comments isn't recursive, so we need to clone/modify the specifiers manually
noDirectCommentsNode.specifiers = (
noDirectCommentsNode.specifiers || []
).map((s) => removeComments(s));
}
return noDirectCommentsNode;
});

if (firstNodesComments) {
addComments(finalNodesClone[0], 'leading', firstNodesComments);
}
attachCommentsToOutputNodes(registry, finalNodesClone);

return finalNodesClone;
};
21 changes: 20 additions & 1 deletion src/utils/get-all-comments-from-nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,32 @@ import type {
Statement,
} from '@babel/types';

export const getAllCommentsFromNodes = (nodes: (Directive | Statement)[]) =>
import { SomeSpecifier } from '../types';

export const getAllCommentsFromNodes = (
nodes: (Directive | Statement | SomeSpecifier)[],
) =>
nodes.reduce((acc, node) => {
if (
Array.isArray(node.leadingComments) &&
node.leadingComments.length > 0
) {
acc = [...acc, ...node.leadingComments];
}
if (
Array.isArray(node.innerComments) &&
node.innerComments.length > 0
) {
acc = [...acc, ...node.innerComments];
}
if (
Array.isArray(node.trailingComments) &&
node.trailingComments.length > 0
) {
acc = [...acc, ...node.trailingComments];
}
if (node.type === 'ImportDeclaration') {
acc = [...acc, ...getAllCommentsFromNodes(node.specifiers)];
}
return acc;
}, [] as (CommentBlock | CommentLine)[]);
Loading