Skip to content

Commit

Permalink
Fix: Do not combine default type imports with value imports (#38)
Browse files Browse the repository at this point in the history
This fixes a bug which was causing default type imports to be combined into value imports, which isn't safe to do.  For instance,

```ts
import { ComponentProps, useEffect } from 'react';
import type React from 'react';
```

was turning into

```ts
import { ComponentProps, useEffect, React } from 'react';
```

Which is not correct.  This change prevents default type imports from ever being merged.
  • Loading branch information
IanVS committed Oct 24, 2022
1 parent 91dd235 commit 274468f
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 2 deletions.
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

0 comments on commit 274468f

Please sign in to comment.