From 668adfa19d451c1cdfd8c2d0091f5f8fbe737755 Mon Sep 17 00:00:00 2001 From: Russell Dunphy Date: Mon, 9 Dec 2024 05:35:26 +0000 Subject: [PATCH] Fix type import ordering (#331) * fix type import ordering * Add a snapshot test for the bugfix --- .../get-import-nodes-matched-group.spec.ts | 63 +++++++++++++++++++ src/utils/get-import-nodes-matched-group.ts | 42 ++++++++----- .../__snapshots__/ppsi.spec.js.snap | 19 ++++++ .../import-with-type-imports-together.ts | 6 ++ 4 files changed, 116 insertions(+), 14 deletions(-) create mode 100644 tests/ImportsSeparated/import-with-type-imports-together.ts diff --git a/src/utils/__tests__/get-import-nodes-matched-group.spec.ts b/src/utils/__tests__/get-import-nodes-matched-group.spec.ts index 2912cb02..b585eb48 100644 --- a/src/utils/__tests__/get-import-nodes-matched-group.spec.ts +++ b/src/utils/__tests__/get-import-nodes-matched-group.spec.ts @@ -43,6 +43,69 @@ test('should return correct matched groups', () => { ]); }); +test('should return type imports as part of a matching group if no type-specific group is present', () => { + const code = ` +import type { ExternalType } from 'external-type-module'; +import type { InternalType } from './internal-type-module'; +import { externalFn } from 'external-fn-module'; +import { internalFn } from './internal-fn-module'; + ` + const importNodes = getImportNodes(code,{ + plugins: ['typescript'], + }); + const importOrder = [ + '^[^.].*', + '^[.].*', + ]; + + let matchedGroups: string[] = []; + for (const importNode of importNodes) { + const matchedGroup = getImportNodesMatchedGroup( + importNode, + importOrder, + ); + matchedGroups.push(matchedGroup); + } + expect(matchedGroups).toEqual([ + '^[^.].*', + '^[.].*', + '^[^.].*', + '^[.].*', + ]); +}); + +test('should return type imports as part of a type-specific group even if a matching non-type specific group precedes it', () => { + const code = ` +import type { ExternalType } from 'external-type-module'; +import type { InternalType } from './internal-type-module'; +import { externalFn } from 'external-fn-module'; +import { internalFn } from './internal-fn-module'; + ` + const importNodes = getImportNodes(code, { + plugins: ['typescript'], + }); + const importOrder = [ + '^[^.].*', + '^[.].*', + '^[.].*', + ]; + + let matchedGroups: string[] = []; + for (const importNode of importNodes) { + const matchedGroup = getImportNodesMatchedGroup( + importNode, + importOrder, + ); + matchedGroups.push(matchedGroup); + } + expect(matchedGroups).toEqual([ + '^[^.].*', + '^[.].*', + '^[^.].*', + '^[.].*', + ]); +}); + test('should return THIRD_PARTY_MODULES as matched group with empty order list', () => { const importNodes = getImportNodes(code); const importOrder: string[] = []; diff --git a/src/utils/get-import-nodes-matched-group.ts b/src/utils/get-import-nodes-matched-group.ts index 2173d6f6..2449b80b 100644 --- a/src/utils/get-import-nodes-matched-group.ts +++ b/src/utils/get-import-nodes-matched-group.ts @@ -22,21 +22,35 @@ export const getImportNodesMatchedGroup = ( : new RegExp(group), })); - for (const { group, regExp } of groupWithRegExp) { + // finding the group for non-type imports is easy: it's the first group that matches. + // however, for type imports, we need to make sure that we don't match a non-type group + // that's earlier in the list than a type-specific group that would otherwise match. + // so we need to get all matching groups, look for the first matching _type-specific_ group, + // and if it exists, return it. otherwise, return the first matching group if there is one. + const matchingGroups = groupWithRegExp.filter(({ group, regExp }) => { if ( - (group.startsWith(TYPES_SPECIAL_WORD) && - node.importKind !== 'type') || - (!group.startsWith(TYPES_SPECIAL_WORD) && - node.importKind === 'type') - ) - continue; + group.startsWith(TYPES_SPECIAL_WORD) && + node.importKind !== 'type' + ) { + return false; + } else { + return node.source.value.match(regExp) !== null; + } + }); - const matched = node.source.value.match(regExp) !== null; - if (matched) return group; + if (matchingGroups.length === 0) { + return node.importKind === 'type' && + importOrder.find( + (group) => group === THIRD_PARTY_TYPES_SPECIAL_WORD, + ) + ? THIRD_PARTY_TYPES_SPECIAL_WORD + : THIRD_PARTY_MODULES_SPECIAL_WORD; + } else if (node.importKind !== 'type') { + return matchingGroups[0].group; + } else { + for (const { group } of matchingGroups) { + if (group.startsWith(TYPES_SPECIAL_WORD)) return group; + } + return matchingGroups[0].group; } - - return node.importKind === 'type' && - importOrder.find((group) => group === THIRD_PARTY_TYPES_SPECIAL_WORD) - ? THIRD_PARTY_TYPES_SPECIAL_WORD - : THIRD_PARTY_MODULES_SPECIAL_WORD; }; diff --git a/tests/ImportsSeparated/__snapshots__/ppsi.spec.js.snap b/tests/ImportsSeparated/__snapshots__/ppsi.spec.js.snap index 4dc55697..bb5d393f 100644 --- a/tests/ImportsSeparated/__snapshots__/ppsi.spec.js.snap +++ b/tests/ImportsSeparated/__snapshots__/ppsi.spec.js.snap @@ -61,6 +61,25 @@ export const a = 1; `; +exports[`import-with-type-imports-together.ts - typescript-verify: import-with-type-imports-together.ts 1`] = ` +import { foo } from "@server/foo" +import type { Quux } from "./quux" +import { Link } from "@ui/Link" +import type { Bar } from "@server/bar" +import type { LinkProps } from "@ui/Link/LinkProps" +import { baz } from "./baz" +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +import type { Bar } from "@server/bar"; +import { foo } from "@server/foo"; + +import { Link } from "@ui/Link"; +import type { LinkProps } from "@ui/Link/LinkProps"; + +import { baz } from "./baz"; +import type { Quux } from "./quux"; + +`; + exports[`imports-with-comments.ts - typescript-verify: imports-with-comments.ts 1`] = ` // I am top level comment in this file. // I am second line of top level comment in this file. diff --git a/tests/ImportsSeparated/import-with-type-imports-together.ts b/tests/ImportsSeparated/import-with-type-imports-together.ts new file mode 100644 index 00000000..b6d28272 --- /dev/null +++ b/tests/ImportsSeparated/import-with-type-imports-together.ts @@ -0,0 +1,6 @@ +import { foo } from "@server/foo" +import type { Quux } from "./quux" +import { Link } from "@ui/Link" +import type { Bar } from "@server/bar" +import type { LinkProps } from "@ui/Link/LinkProps" +import { baz } from "./baz"