Don’t touch imports used for module augmentation in non-declaration files since it could change JS emit

andrewbranch · andrewbranch · commit 60d0bb9b199e · 2019-05-22T11:37:35.000-07:00
diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts
@@ -125,13 +125,22 @@ namespace ts.OrganizeImports {
             if (name || namedBindings) {
                 usedImports.push(updateImportDeclarationAndClause(importDecl, name, namedBindings));
             }
-            // If a module is imported to be augmented, keep the import declaration, but without an import clause
+            // If a module is imported to be augmented, it’s used
             else if (hasModuleDeclarationMatchingSpecifier(sourceFile, moduleSpecifier)) {
-                usedImports.push(createImportDeclaration(
-                    importDecl.decorators,
-                    importDecl.modifiers,
-                    /*importClause*/ undefined,
-                    moduleSpecifier));
+                // If we’re in a declaration file, it’s safe to remove the import clause from it
+                if (sourceFile.isDeclarationFile) {
+                    usedImports.push(createImportDeclaration(
+                        importDecl.decorators,
+                        importDecl.modifiers,
+                        /*importClause*/ undefined,
+                        moduleSpecifier));
+                }
+                // If we’re not in a declaration file, we can’t remove the import clause even though
+                // the imported symbols are unused, because removing them makes it look like the import
+                // declaration has side effects, which will cause it to be preserved in the JS emit.
+                else {
+                    usedImports.push(importDecl);
+                }
             }
         }
 
@@ -145,10 +154,9 @@ namespace ts.OrganizeImports {
 
     function hasModuleDeclarationMatchingSpecifier(sourceFile: SourceFile, moduleSpecifier: Expression) {
         const moduleSpecifierText = isStringLiteral(moduleSpecifier) && moduleSpecifier.text;
-        return isString(moduleSpecifierText) && some(sourceFile.statements, statement =>
-            isModuleDeclaration(statement)
-            && isStringLiteral(statement.name)
-            && statement.name.text === moduleSpecifierText);
+        return isString(moduleSpecifierText) && some(sourceFile.moduleAugmentations, moduleName =>
+            isStringLiteral(moduleName)
+            && moduleName.text === moduleSpecifierText);
     }
 
     function getExternalModuleName(specifier: Expression) {
diff --git a/src/testRunner/unittests/services/organizeImports.ts b/src/testRunner/unittests/services/organizeImports.ts
@@ -350,6 +350,21 @@ import { } from "lib";
 import foo from 'foo';
 import { Caseless } from 'caseless';
 
+declare module 'foo' {}
+declare module 'caseless' {
+    interface Caseless {
+        test(name: KeyType): boolean;
+    }
+}`
+            });
+
+            testOrganizeImports("Unused_preserve_imports_for_module_augmentation_in_non_declaration_file",
+            {
+                path: "/test.ts",
+                content: `
+import foo from 'foo';
+import { Caseless } from 'caseless';
+
 declare module 'foo' {}
 declare module 'caseless' {
     interface Caseless {
diff --git a/tests/baselines/reference/organizeImports/Unused_preserve_imports_for_module_augmentation_in_non_declaration_file.ts b/tests/baselines/reference/organizeImports/Unused_preserve_imports_for_module_augmentation_in_non_declaration_file.ts
@@ -0,0 +1,22 @@
+// ==ORIGINAL==
+
+import foo from 'foo';
+import { Caseless } from 'caseless';
+
+declare module 'foo' {}
+declare module 'caseless' {
+    interface Caseless {
+        test(name: KeyType): boolean;
+    }
+}
+// ==ORGANIZED==
+
+import { Caseless } from 'caseless';
+import foo from 'foo';
+
+declare module 'foo' {}
+declare module 'caseless' {
+    interface Caseless {
+        test(name: KeyType): boolean;
+    }
+}
