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: Do not combine default type imports with value imports #38

Merged
merged 2 commits into from Oct 24, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion README.md
Expand Up @@ -228,7 +228,17 @@ _Note:_ If you want greater control over which groups are separated from others,

**default value:** `false`

A boolean value to enable or disable sorting of the specifiers in an import declarations.
A boolean value to enable or disable sorting of the specifiers in an import declarations. If enabled, type imports will be sorted after value imports.

Before:
```ts
import Default, {type Bravo, delta as echo, charlie, type Alpha} from 'source';
```

After:
```ts
import Default, {charlie, delta as echo, type Alpha, type Bravo} from 'source';
```

#### `importOrderGroupNamespaceSpecifiers`

Expand Down
35 changes: 35 additions & 0 deletions src/utils/__tests__/merge-nodes-with-matching-flavors.spec.ts
Expand Up @@ -88,6 +88,7 @@ import Foo2 from "e";
import { Junk2 } from "junk-group-2";
`);
});

it('should merge type imports into regular imports', () => {
const code = `
// Preserves 'import type'
Expand Down Expand Up @@ -130,6 +131,7 @@ import { C2, type C1 } from "c";
import { D1, type D2 } from "d";
`);
});

it('should combine type import and default import', () => {
const code = `
import type {MyType} from './source';
Expand All @@ -155,6 +157,7 @@ import defaultValue from './source';
.toEqual(`import defaultValue, { type MyType } from "./source";
`);
});

it('should not combine type import and namespace import', () => {
const code = `
import type {MyType} from './source';
Expand All @@ -181,6 +184,7 @@ import * as Namespace from './source';
import * as Namespace from "./source";
`);
});

it('should support aliased named imports', () => {
const code = `
import type {MyType} from './source';
Expand All @@ -206,6 +210,7 @@ import {value as alias} from './source';
.toEqual(`import { value as alias, type MyType } from "./source";
`);
});

it('should combine multiple imports from the same source', () => {
const code = `
import type {MyType, SecondType} from './source';
Expand All @@ -231,6 +236,7 @@ import {value, SecondValue} from './source';
.toEqual(`import { SecondValue, value, type MyType, type SecondType } from "./source";
`);
});

it('should combine multiple groups of imports', () => {
const code = `
import type {MyType} from './source';
Expand Down Expand Up @@ -259,6 +265,7 @@ import {otherValue} from './other';
import { value, type MyType } from "./source";
`);
});

it('should combine multiple imports statements from the same source', () => {
const code = `
import type {MyType} from './source';
Expand Down Expand Up @@ -286,6 +293,7 @@ import {SecondValue} from './source';
.toEqual(`import { SecondValue, value, type MyType, type SecondType } from "./source";
`);
});

it('should not impact imports from different sources', () => {
const code = `
import type {MyType} from './source';
Expand Down Expand Up @@ -392,3 +400,30 @@ import Foo2 from "e";
import { Junk2 } from "junk-group-2";
`);
});

it('should not combine default type imports', () => {
const code = `
import { ComponentProps, useEffect } from "react";
import type React from "react";
`;
const allOriginalImportNodes = getImportNodes(code, {
plugins: ['typescript'],
});

const nodesToOutput = getSortedNodes(allOriginalImportNodes, {
...defaultOptions,
importOrderMergeDuplicateImports: true,
importOrderCombineTypeAndValueImports: true,
});
const formatted = getCodeFromAst({
nodesToOutput,
allOriginalImportNodes,
originalCode: code,
directives: [],
});

expect(format(formatted, { parser: 'babel' }))
.toEqual(`import { ComponentProps, useEffect } from "react";
import type React from "react";
`);
});
15 changes: 14 additions & 1 deletion src/utils/merge-nodes-with-matching-flavors.ts
Expand Up @@ -57,7 +57,13 @@ function nodeIsImportNamespaceSpecifier(
): node is ImportNamespaceSpecifier {
return node.type === 'ImportNamespaceSpecifier';
}
/** e.g. import Default from "someModule" */
/**
* Default type or value import
*
* e.g.
* import Default from "someModule"
* import type Default from "someModule"
*/
function nodeIsImportDefaultSpecifier(
node: ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier,
): node is ImportDefaultSpecifier {
Expand Down Expand Up @@ -108,6 +114,13 @@ function mergeIsSafe(
// But since this runs the risk of making code longer, this won't be in v1.
return false;
}
if (
nodeToKeep.importKind === 'type' && nodeToKeep.specifiers.some(nodeIsImportDefaultSpecifier) ||
nodeToForget.importKind === 'type' && nodeToForget.specifiers.some(nodeIsImportDefaultSpecifier)
) {
// Cannot merge default type imports (.e.g. import type React from 'react')
return false;
}
return true;
}

Expand Down