From 274468fe54c26135717761746c649bc5e2697a76 Mon Sep 17 00:00:00 2001
From: Ian VanSchooten <ian.vanschooten@gmail.com>
Date: Mon, 24 Oct 2022 09:44:52 -0400
Subject: [PATCH] Fix: Do not combine default type imports with value imports
 (#38)

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.
---
 README.md                                     | 12 ++++++-
 .../merge-nodes-with-matching-flavors.spec.ts | 35 +++++++++++++++++++
 .../merge-nodes-with-matching-flavors.ts      | 15 +++++++-
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index 9f93ed4..237b369 100644
--- a/README.md
+++ b/README.md
@@ -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`
 
diff --git a/src/utils/__tests__/merge-nodes-with-matching-flavors.spec.ts b/src/utils/__tests__/merge-nodes-with-matching-flavors.spec.ts
index 922fe5d..2edf563 100644
--- a/src/utils/__tests__/merge-nodes-with-matching-flavors.spec.ts
+++ b/src/utils/__tests__/merge-nodes-with-matching-flavors.spec.ts
@@ -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'
@@ -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';
@@ -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';
@@ -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';
@@ -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';
@@ -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';
@@ -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';
@@ -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';
@@ -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";
+`);
+});
diff --git a/src/utils/merge-nodes-with-matching-flavors.ts b/src/utils/merge-nodes-with-matching-flavors.ts
index 5a760e3..83d974d 100644
--- a/src/utils/merge-nodes-with-matching-flavors.ts
+++ b/src/utils/merge-nodes-with-matching-flavors.ts
@@ -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 {
@@ -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;
 }