Suggestion to convert to ES6 module should only trigger in projects which express some intent to use ES6 (microsoft#23576)

Andy · web-flow · commit fc30bd10e33e · 2018-04-23T16:05:37.000-07:00
diff --git a/src/services/completions.ts b/src/services/completions.ts
@@ -1201,14 +1201,11 @@ namespace ts.Completions {
             // If already using commonjs, don't introduce ES6.
             if (sourceFile.commonJsModuleIndicator) return false;
             // If some file is using ES6 modules, assume that it's OK to add more.
-            if (program.getSourceFiles().some(s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s) && !!s.externalModuleIndicator)) {
-                return true;
-            }
+            if (programContainsEs6Modules(program)) return true;
             // For JS, stay on the safe side.
             if (isSourceFileJavaScript(sourceFile)) return false;
             // If module transpilation is enabled or we're targeting es6 or above, or not emitting, OK.
-            const compilerOptions = program.getCompilerOptions();
-            return !!compilerOptions.module || compilerOptions.target >= ScriptTarget.ES2015 || !!compilerOptions.noEmit;
+            return compilerOptionsIndicateEs6Modules(program.getCompilerOptions());
         }
 
         function isSnippetScope(scopeNode: Node): boolean {
diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts
@@ -5,7 +5,7 @@ namespace ts {
         const checker = program.getDiagnosticsProducingTypeChecker();
         const diags: Diagnostic[] = [];
 
-        if (sourceFile.commonJsModuleIndicator) {
+        if (sourceFile.commonJsModuleIndicator && (programContainsEs6Modules(program) || compilerOptionsIndicateEs6Modules(program.getCompilerOptions()))) {
             diags.push(createDiagnosticForNode(getErrorNodeFromCommonJsIndicator(sourceFile.commonJsModuleIndicator), Diagnostics.File_is_a_CommonJS_module_it_may_be_converted_to_an_ES6_module));
         }
 
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -1214,6 +1214,13 @@ namespace ts {
             : getTextOfIdentifierOrLiteral(name);
     }
 
+    export function programContainsEs6Modules(program: Program): boolean {
+        return program.getSourceFiles().some(s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s) && !!s.externalModuleIndicator);
+    }
+    export function compilerOptionsIndicateEs6Modules(compilerOptions: CompilerOptions): boolean {
+        return !!compilerOptions.module || compilerOptions.target >= ScriptTarget.ES2015 || !!compilerOptions.noEmit;
+    }
+
     export function hostUsesCaseSensitiveFileNames(host: LanguageServiceHost): boolean {
         return host.useCaseSensitiveFileNames ? host.useCaseSensitiveFileNames() : false;
     }
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const exportsAlias = exports;
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_dotDefault.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_dotDefault.ts
@@ -3,6 +3,7 @@
 // Test that we leave it alone if the name is a keyword.
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////exports.default = 0;
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_invalidName.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_invalidName.ts
@@ -3,6 +3,7 @@
 // Test that we leave it alone if the name is a keyword.
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////exports.class = 0;
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExports.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExports.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////module.exports = function() {}
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExportsEqualsRequire.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExportsEqualsRequire.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.d.ts
 ////export const x: number;
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExports_changesImports.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExports_changesImports.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////module.exports = 0;
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////[|exports.f|] = function() {};
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////exports.C = class E { static instance = new E(); }
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////exports.f = function g() { g(); }
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_object.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_object.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////module.exports = {
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_object_shorthand.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_object_shorthand.ts
@@ -3,6 +3,7 @@
 // TODO: Maybe we could transform this to `export function f() {}`.
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////function f() {}
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////exports.x = 0;
@@ -15,8 +16,6 @@
 ////    exports.z; z;
 ////}
 
-// TODO: GH#22492 Should be a able access `exports.z` inside `exports.f`
-
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts b/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////exports.f = async function* f(p) { p; }
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_arrayBindingPattern.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_arrayBindingPattern.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const [x, y] = /*a*/require/*b*/("x");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_includeDefaultUses.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_includeDefaultUses.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const x = /*a*/require/*b*/("x");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleUniqueIdentifiers.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleUniqueIdentifiers.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const x = require("x");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleVariableDeclarations.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleVariableDeclarations.ts
@@ -3,6 +3,7 @@
 // Test that we leave it alone if the name is a keyword.
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const x = require("x"), y = 0, { z } = require("z");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_nameFromModuleSpecifier.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_nameFromModuleSpecifier.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const [] = require("a-b");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_complex.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_complex.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const { x: { a, b } } = require("x");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_plain.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_plain.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const { x, y: z } = require("x");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_onlyNamedImports.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_onlyNamedImports.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const x = require("x");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_propertyAccess.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_propertyAccess.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const x = require("x").default;
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_shadowing.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_shadowing.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////const mod = require("mod");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_sideEffect.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_sideEffect.ts
@@ -3,6 +3,7 @@
 // Test that we leave it alone if the name is a keyword.
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////require("foo");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_missingInitializer.ts b/tests/cases/fourslash/refactorConvertToEs6Module_missingInitializer.ts
@@ -1,6 +1,7 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
 
 // @Filename: /a.js
 ////require("m");
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_notInCommonjsProject.ts b/tests/cases/fourslash/refactorConvertToEs6Module_notInCommonjsProject.ts
@@ -0,0 +1,10 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+
+// @Filename: /a.js
+////exports.x = 0;
+
+// No suggestion to convert to an ES6 module,
+// since there are no es6 modules in this project and we don't have a high enough target.
+verify.getSuggestionDiagnostics([]);
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_notInCommonjsProject_yesIfSomeEs6Module.ts b/tests/cases/fourslash/refactorConvertToEs6Module_notInCommonjsProject_yesIfSomeEs6Module.ts
@@ -0,0 +1,14 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+
+// @Filename: /a.js
+////exports.x = 0;
+
+// @Filename: /b.js
+////export const y = 0;
+
+verify.codeFix({
+    description: "Convert to ES6 module",
+    newFileContent: "export const x = 0;",
+});
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_preserveQuotes.ts b/tests/cases/fourslash/refactorConvertToEs6Module_preserveQuotes.ts
@@ -1,6 +1,8 @@
 /// <reference path='fourslash.ts' />
 
 // @allowJs: true
+// @target: esnext
+
 // @Filename: /a.js
 ////const a = require('a'); a;
 

Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ namespace ts {`
`5`	`5`	`const checker = program.getDiagnosticsProducingTypeChecker();`
`6`	`6`	`const diags: Diagnostic[] = [];`
`7`	`7`
`8`		`- if (sourceFile.commonJsModuleIndicator) {`
	`8`	`+ if (sourceFile.commonJsModuleIndicator && (programContainsEs6Modules(program) \|\| compilerOptionsIndicateEs6Modules(program.getCompilerOptions()))) {`
`9`	`9`	`diags.push(createDiagnosticForNode(getErrorNodeFromCommonJsIndicator(sourceFile.commonJsModuleIndicator), Diagnostics.File_is_a_CommonJS_module_it_may_be_converted_to_an_ES6_module));`
`10`	`10`	`}`
`11`	`11`
Original file line number	Diff line number	Diff line change
`@@ -1214,6 +1214,13 @@ namespace ts {`
`1214`	`1214`	`: getTextOfIdentifierOrLiteral(name);`
`1215`	`1215`	`}`
`1216`	`1216`
	`1217`	`+ export function programContainsEs6Modules(program: Program): boolean {`
	`1218`	`+ return program.getSourceFiles().some(s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s) && !!s.externalModuleIndicator);`
	`1219`	`+ }`
	`1220`	`+ export function compilerOptionsIndicateEs6Modules(compilerOptions: CompilerOptions): boolean {`
	`1221`	`+ return !!compilerOptions.module \|\| compilerOptions.target >= ScriptTarget.ES2015 \|\| !!compilerOptions.noEmit;`
	`1222`	`+ }`
	`1223`	`+`
`1217`	`1224`	`export function hostUsesCaseSensitiveFileNames(host: LanguageServiceHost): boolean {`
`1218`	`1225`	`return host.useCaseSensitiveFileNames ? host.useCaseSensitiveFileNames() : false;`
`1219`	`1226`	`}`