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 20 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,17 @@ 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 */`).

### Commented Lines inside of Imports

We make the following attempts at keeping comments in your imports clean, but there's some pain points when imports are shuffled.

Specific cases we handle:

- If you have one or more comments that end above the line immediately preceding your first import, we will treat them as top-of-file comment(s) and avoid moving them when the first-import moves down.
- If you have comments that come after after your last import; those comments and following code will stay below the imports. (Runtime-code between imports will be moved below all the imports)
- In general, if you place a single-line comment on the same line as an Import `Declaration` or `*Specifier`, we will keep it attached to that same specifier if that line moves around (due to mergers, or sorting changes due to newly inserted imports).
- Other comments are preserved, and are generally considered `leadingComments` 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
44 changes: 22 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,13 @@ 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(trailingComments(adjustedNodes[0])).toEqual([]);
expect(leadingComments(adjustedNodes[1])).toEqual([' comment b']);
expect(adjustedNodes).toHaveLength(4);
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([]);
expect(leadingComments(adjustedNodes[3])).toEqual([]);
expect(trailingComments(adjustedNodes[3])).toEqual([]);
});

test('it preserves multiple leading comments for each import declaration', () => {
Expand All @@ -47,21 +47,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(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', () => {
Expand All @@ -75,16 +75,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 explicitly detached so it stays with the top-of-file
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't comment c1 move? In your note in the readme, it says"

If you leave a gap after a comment at the top of your file, we will avoid moving it around if the imports below it shift.

So I would assume since there's no gap, that c1 and c2 would both move with the import from "c".

Copy link
Collaborator Author

@fbartho fbartho May 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I see how I confused things particularly with my variable names and the term “gap”.

I didn’t mean “gap” as in blank line — I have to consider each comment independently (or I need even more richness in my data model/scan-passes). — So gap is “if this comment and the import-node were the only things that existed, would there be a gap?”

So “leading gap” really was “is this specific 1-line comment ending more than 1 line before the import.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the variables and comments, in particular because the change from #71 (comment) also further twisted the meaning of one of them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this makes sense, thanks. It seems like a pretty rare thing to happen, and has an easy solution if someone wants both to stay at the top (add a blank line underneath).

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 +98,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
55 changes: 32 additions & 23 deletions src/utils/adjust-comments-on-sorted-nodes.ts
Original file line number Diff line number Diff line change
@@ -1,43 +1,52 @@
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],
lastImport:
originalDeclarationNodes[originalDeclarationNodes.length - 1],
});

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)[]);
13 changes: 6 additions & 7 deletions src/utils/get-code-from-ast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
type Statement,
} from '@babel/types';

import { newLineCharacters } from '../constants';
import { injectNewlinesRegex, newLineCharacters } from '../constants';
import { getAllCommentsFromNodes } from './get-all-comments-from-nodes';
import { removeNodesFromOriginalCode } from './remove-nodes-from-original-code';

Expand Down Expand Up @@ -70,10 +70,9 @@ export const getCodeFromAst = ({

const { code } = generate(newAST);

return (
code.replace(
/"PRETTIER_PLUGIN_SORT_IMPORTS_NEW_LINE";/gi,
newLineCharacters,
) + codeWithoutImportsAndInterpreter.trim()
);
const replacedCode = code.replace(injectNewlinesRegex, newLineCharacters);

const trailingCode = codeWithoutImportsAndInterpreter.trim();

return replacedCode + trailingCode;
};