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 type import ordering #331

Merged
merged 2 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
63 changes: 63 additions & 0 deletions src/utils/__tests__/get-import-nodes-matched-group.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
'^[^.].*',
'^[.].*',
'<TS_TYPES>^[.].*',
];

let matchedGroups: string[] = [];
for (const importNode of importNodes) {
const matchedGroup = getImportNodesMatchedGroup(
importNode,
importOrder,
);
matchedGroups.push(matchedGroup);
}
expect(matchedGroups).toEqual([
'^[^.].*',
'<TS_TYPES>^[.].*',
'^[^.].*',
'^[.].*',
]);
});

test('should return THIRD_PARTY_MODULES as matched group with empty order list', () => {
const importNodes = getImportNodes(code);
const importOrder: string[] = [];
Expand Down
42 changes: 28 additions & 14 deletions src/utils/get-import-nodes-matched-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
19 changes: 19 additions & 0 deletions tests/ImportsSeparated/__snapshots__/ppsi.spec.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions tests/ImportsSeparated/import-with-type-imports-together.ts
Original file line number Diff line number Diff line change
@@ -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"
Loading