From dcbadcb05a3924218bfd7eaa32e324f83cd79b36 Mon Sep 17 00:00:00 2001 From: Aluan Haddad Date: Sat, 28 Oct 2017 08:08:56 -0400 Subject: [PATCH 1/6] improve import code fixes for UMD modules - use default import under --allowSyntheticDefaultImports - import..require support - make make quick fix info match resulting import - make diagnostics --- src/compiler/diagnosticMessages.json | 8 ++++ src/services/codefixes/importFixes.ts | 56 +++++++++++++++++++++------ 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index d3669ce397dec..60e3d6c4c4fc6 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3801,5 +3801,13 @@ "Install '{0}'": { "category": "Message", "code": 95014 + }, + "Import '{0}' = require(\"{1}\").": { + "category": "Message", + "code": 95015 + }, + "Import * as '{0}' from \"{1}\".": { + "category": "Message", + "code": 95016 } } diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 3f0d27e5b070f..dfc770cf3aacb 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -181,6 +181,7 @@ namespace ts.codefix { Named, Default, Namespace, + Equals } export function getCodeActionForImport(moduleSymbol: Symbol, context: ImportCodeFixOptions): ImportCodeAction[] { @@ -212,7 +213,7 @@ namespace ts.codefix { function getNamespaceImportName(declaration: AnyImportSyntax): Identifier { if (declaration.kind === SyntaxKind.ImportDeclaration) { - const namedBindings = declaration.importClause && declaration.importClause.namedBindings; + const namedBindings = declaration.importClause && isImportClause(declaration.importClause) && declaration.importClause.namedBindings; return namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport ? namedBindings.name : undefined; } else { @@ -237,6 +238,8 @@ namespace ts.codefix { return parent as ImportDeclaration; case SyntaxKind.ExternalModuleReference: return (parent as ExternalModuleReference).parent; + case SyntaxKind.ImportEqualsDeclaration: + return parent as ImportEqualsDeclaration; default: Debug.assert(parent.kind === SyntaxKind.ExportDeclaration); // Ignore these, can't add imports to them. @@ -249,11 +252,19 @@ namespace ts.codefix { const lastImportDeclaration = findLast(sourceFile.statements, isAnyImportSyntax); const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier); - const importDecl = createImportDeclaration( - /*decorators*/ undefined, - /*modifiers*/ undefined, - createImportClauseOfKind(kind, symbolName), - createStringLiteralWithQuoteStyle(sourceFile, moduleSpecifierWithoutQuotes)); + const quotedModuleSpecifier = createStringLiteralWithQuoteStyle(sourceFile, moduleSpecifierWithoutQuotes); + const importDecl = kind !== ImportKind.Equals + ? createImportDeclaration( + /*decorators*/ undefined, + /*modifiers*/ undefined, + createImportClauseOfKind(kind, symbolName), + quotedModuleSpecifier) + : createImportEqualsDeclaration( + /*decorators*/ undefined, + /*modifiers*/ undefined, + createIdentifier(symbolName), + createExternalModuleReference(quotedModuleSpecifier)); + const changes = ChangeTracker.with(context, changeTracker => { if (lastImportDeclaration) { changeTracker.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl, { suffix: newLineCharacter }); @@ -263,11 +274,17 @@ namespace ts.codefix { } }); + const actionFormat = kind === ImportKind.Equals + ? Diagnostics.Import_0_require_1 + : kind === ImportKind.Namespace + ? Diagnostics.Import_Asterisk_as_0_from_1 + : Diagnostics.Import_0_from_1; + // if this file doesn't have any import statements, insert an import statement and then insert a new line // between the only import statement and user code. Otherwise just insert the statement because chances // are there are already a new line seperating code and import statements. return createCodeAction( - Diagnostics.Import_0_from_1, + actionFormat, [symbolName, moduleSpecifierWithoutQuotes], changes, "NewImport", @@ -282,7 +299,7 @@ namespace ts.codefix { return literal; } - function createImportClauseOfKind(kind: ImportKind, symbolName: string) { + function createImportClauseOfKind(kind: ImportKind.Default | ImportKind.Named | ImportKind.Namespace, symbolName: string) { const id = createIdentifier(symbolName); switch (kind) { case ImportKind.Default: @@ -534,7 +551,7 @@ namespace ts.codefix { declarations: ReadonlyArray): ImportCodeAction { const fromExistingImport = firstDefined(declarations, declaration => { if (declaration.kind === SyntaxKind.ImportDeclaration && declaration.importClause) { - const changes = tryUpdateExistingImport(ctx, declaration.importClause); + const changes = tryUpdateExistingImport(ctx, isImportClause(declaration.importClause) && declaration.importClause || undefined); if (changes) { const moduleSpecifierWithoutQuotes = stripQuotes(declaration.moduleSpecifier.getText()); return createCodeAction( @@ -564,9 +581,10 @@ namespace ts.codefix { return expression && isStringLiteral(expression) ? expression.text : undefined; } - function tryUpdateExistingImport(context: SymbolContext & { kind: ImportKind }, importClause: ImportClause): FileTextChanges[] | undefined { + function tryUpdateExistingImport(context: SymbolContext & { kind: ImportKind }, importClause: ImportClause | ImportEqualsDeclaration): FileTextChanges[] | undefined { const { symbolName, sourceFile, kind } = context; - const { name, namedBindings } = importClause; + const { name } = importClause; + const { namedBindings } = importClause.kind !== SyntaxKind.ImportEqualsDeclaration && importClause; switch (kind) { case ImportKind.Default: return name ? undefined : ChangeTracker.with(context, t => @@ -592,6 +610,9 @@ namespace ts.codefix { return namedBindings ? undefined : ChangeTracker.with(context, t => t.replaceNode(sourceFile, importClause, createImportClause(name, createNamespaceImport(createIdentifier(symbolName))))); + case ImportKind.Equals: + return undefined; + default: Debug.assertNever(kind); } @@ -644,6 +665,19 @@ namespace ts.codefix { Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here"); } + const { module, allowSyntheticDefaultImports } = context.compilerOptions; + + // Prefer to import as a synthetic `default` if available. + if (allowSyntheticDefaultImports || module === ModuleKind.System && allowSyntheticDefaultImports !== false) { + return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Default }); + } + + // When a synthetic `default` is unavailable, use `import..require` if the module kind supports it. + if (module === ModuleKind.AMD || module === ModuleKind.CommonJS || module === ModuleKind.UMD) { + return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Equals }); + } + + // Fall back to `* as ns` style imports. return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Namespace }); } From 3041e10080587902592e1c11ec453df8effdd40b Mon Sep 17 00:00:00 2001 From: Aluan Haddad Date: Wed, 1 Nov 2017 11:17:12 -0400 Subject: [PATCH 2/6] Address PR feedback: - extract test for synethetic default imports into getAllowSyntheticDefaultImports in core.ts - use getAllowSyntheticDefaultImports in checker.ts and importFixes.ts - move compilerOptions to top level destructuring --- src/compiler/checker.ts | 2 +- src/compiler/core.ts | 7 +++++++ src/services/codefixes/importFixes.ts | 9 +++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index e2e45b2ccb0f6..c937160ae7277 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -64,7 +64,7 @@ namespace ts { const languageVersion = getEmitScriptTarget(compilerOptions); const modulekind = getEmitModuleKind(compilerOptions); const noUnusedIdentifiers = !!compilerOptions.noUnusedLocals || !!compilerOptions.noUnusedParameters; - const allowSyntheticDefaultImports = typeof compilerOptions.allowSyntheticDefaultImports !== "undefined" ? compilerOptions.allowSyntheticDefaultImports : modulekind === ModuleKind.System; + const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); const strictNullChecks = getStrictOptionValue(compilerOptions, "strictNullChecks"); const strictFunctionTypes = getStrictOptionValue(compilerOptions, "strictFunctionTypes"); const noImplicitAny = getStrictOptionValue(compilerOptions, "noImplicitAny"); diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 157a0c7fd1d3e..84b8c876863df 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -1683,6 +1683,13 @@ namespace ts { return moduleResolution; } + export function getAllowSyntheticDefaultImports(compilerOptions: CompilerOptions) { + const moduleKind = getEmitModuleKind(compilerOptions); + return compilerOptions.allowSyntheticDefaultImports !== undefined + ? compilerOptions.allowSyntheticDefaultImports + : moduleKind === ModuleKind.System; + } + export type StrictOptionName = "noImplicitAny" | "noImplicitThis" | "strictNullChecks" | "strictFunctionTypes" | "alwaysStrict"; export function getStrictOptionValue(compilerOptions: CompilerOptions, flag: StrictOptionName): boolean { diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index dfc770cf3aacb..c50da337eac18 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -648,7 +648,7 @@ namespace ts.codefix { } function getActionsForUMDImport(context: ImportCodeFixContext): ImportCodeAction[] { - const { checker, symbolToken } = context; + const { checker, symbolToken, compilerOptions } = context; const umdSymbol = checker.getSymbolAtLocation(symbolToken); let symbol: ts.Symbol; let symbolName: string; @@ -665,15 +665,16 @@ namespace ts.codefix { Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here"); } - const { module, allowSyntheticDefaultImports } = context.compilerOptions; + const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); // Prefer to import as a synthetic `default` if available. - if (allowSyntheticDefaultImports || module === ModuleKind.System && allowSyntheticDefaultImports !== false) { + if (allowSyntheticDefaultImports) { return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Default }); } + const moduleKind = getEmitModuleKind(compilerOptions); // When a synthetic `default` is unavailable, use `import..require` if the module kind supports it. - if (module === ModuleKind.AMD || module === ModuleKind.CommonJS || module === ModuleKind.UMD) { + if (moduleKind === ModuleKind.AMD || moduleKind === ModuleKind.CommonJS || moduleKind === ModuleKind.UMD) { return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Equals }); } From 1be8b2131eb71d9aa3a46716e00c399a1985b9bc Mon Sep 17 00:00:00 2001 From: Aluan Haddad Date: Wed, 1 Nov 2017 14:45:57 -0400 Subject: [PATCH 3/6] add tests --- ...xNewImportAllowSyntheticDefaultImports0.ts | 18 ++++++++++++++++++ ...xNewImportAllowSyntheticDefaultImports1.ts | 19 +++++++++++++++++++ ...xNewImportAllowSyntheticDefaultImports2.ts | 19 +++++++++++++++++++ ...xNewImportAllowSyntheticDefaultImports3.ts | 18 ++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports0.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports0.ts new file mode 100644 index 0000000000000..3b62e0e9d5f7d --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports0.ts @@ -0,0 +1,18 @@ +/// +// @AllowSyntheticDefaultImports: true + +// @Filename: a/f1.ts +//// [|export var x = 0; +//// bar/*0*/();|] + +// @Filename: a/foo.d.ts +//// declare function bar(): number; +//// export = bar; +//// export as namespace bar; + +verify.importFixAtPosition([ +`import bar from "./foo"; + +export var x = 0; +bar();` + ]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts new file mode 100644 index 0000000000000..d881a97968d5a --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts @@ -0,0 +1,19 @@ +/// +// @Module: es2015 +// @AllowSyntheticDefaultImports: false + +// @Filename: a/f1.ts +//// [|export var x = 0; +//// bar/*0*/();|] + +// @Filename: a/foo.d.ts +//// declare function bar(): number; +//// export = bar; +//// export as namespace bar; + +verify.importFixAtPosition([ +`import * as bar from "./foo"; + +export var x = 0; +bar();` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts new file mode 100644 index 0000000000000..72f6fcd2d1585 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts @@ -0,0 +1,19 @@ +/// +// @Module: commonjs +// @AllowSyntheticDefaultImports: false + +// @Filename: a/f1.ts +//// [|export var x = 0; +//// bar/*0*/();|] + +// @Filename: a/foo.d.ts +//// declare function bar(): number; +//// export = bar; +//// export as namespace bar; + +verify.importFixAtPosition([ +`import bar = require("./foo"); + +export var x = 0; +bar();` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts new file mode 100644 index 0000000000000..c6c2a5548741c --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts @@ -0,0 +1,18 @@ +/// +// @Module: system + +// @Filename: a/f1.ts +//// [|export var x = 0; +//// bar/*0*/();|] + +// @Filename: a/foo.d.ts +//// declare function bar(): number; +//// export = bar; +//// export as namespace bar; + +verify.importFixAtPosition([ +`import bar from "./foo"; + +export var x = 0; +bar();` +]); \ No newline at end of file From f27148d7968be9f37507b4509f98040414ef141c Mon Sep 17 00:00:00 2001 From: Aluan Haddad Date: Mon, 6 Nov 2017 18:56:24 -0500 Subject: [PATCH 4/6] remove `import =` quick fix and supporting code. --- src/services/codefixes/importFixes.ts | 34 +++++++-------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index c50da337eac18..73cedc883be7d 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -180,8 +180,7 @@ namespace ts.codefix { export const enum ImportKind { Named, Default, - Namespace, - Equals + Namespace } export function getCodeActionForImport(moduleSymbol: Symbol, context: ImportCodeFixOptions): ImportCodeAction[] { @@ -253,17 +252,11 @@ namespace ts.codefix { const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier); const quotedModuleSpecifier = createStringLiteralWithQuoteStyle(sourceFile, moduleSpecifierWithoutQuotes); - const importDecl = kind !== ImportKind.Equals - ? createImportDeclaration( + const importDecl = createImportDeclaration( /*decorators*/ undefined, /*modifiers*/ undefined, - createImportClauseOfKind(kind, symbolName), - quotedModuleSpecifier) - : createImportEqualsDeclaration( - /*decorators*/ undefined, - /*modifiers*/ undefined, - createIdentifier(symbolName), - createExternalModuleReference(quotedModuleSpecifier)); + createImportClauseOfKind(kind, symbolName), + quotedModuleSpecifier); const changes = ChangeTracker.with(context, changeTracker => { if (lastImportDeclaration) { @@ -274,10 +267,8 @@ namespace ts.codefix { } }); - const actionFormat = kind === ImportKind.Equals - ? Diagnostics.Import_0_require_1 - : kind === ImportKind.Namespace - ? Diagnostics.Import_Asterisk_as_0_from_1 + const actionFormat = kind === ImportKind.Namespace + ? Diagnostics.Import_Asterisk_as_0_from_1 : Diagnostics.Import_0_from_1; // if this file doesn't have any import statements, insert an import statement and then insert a new line @@ -610,9 +601,6 @@ namespace ts.codefix { return namedBindings ? undefined : ChangeTracker.with(context, t => t.replaceNode(sourceFile, importClause, createImportClause(name, createNamespaceImport(createIdentifier(symbolName))))); - case ImportKind.Equals: - return undefined; - default: Debug.assertNever(kind); } @@ -667,18 +655,12 @@ namespace ts.codefix { const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); - // Prefer to import as a synthetic `default` if available. + // Import a synthetic `default` if enabled. if (allowSyntheticDefaultImports) { return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Default }); } - const moduleKind = getEmitModuleKind(compilerOptions); - - // When a synthetic `default` is unavailable, use `import..require` if the module kind supports it. - if (moduleKind === ModuleKind.AMD || moduleKind === ModuleKind.CommonJS || moduleKind === ModuleKind.UMD) { - return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Equals }); - } - // Fall back to `* as ns` style imports. + // Fall back to the `import * as ns` style import. return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Namespace }); } From 78ee07fcb7be599251d9dbcb7c413b42f80c75dc Mon Sep 17 00:00:00 2001 From: Aluan Haddad Date: Mon, 6 Nov 2017 18:59:56 -0500 Subject: [PATCH 5/6] update feature tests --- ...ixNewImportAllowSyntheticDefaultImports1.ts | 5 ++--- ...ixNewImportAllowSyntheticDefaultImports2.ts | 3 +-- ...ixNewImportAllowSyntheticDefaultImports3.ts | 18 ------------------ 3 files changed, 3 insertions(+), 23 deletions(-) delete mode 100644 tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts index d881a97968d5a..c6c2a5548741c 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports1.ts @@ -1,6 +1,5 @@ /// -// @Module: es2015 -// @AllowSyntheticDefaultImports: false +// @Module: system // @Filename: a/f1.ts //// [|export var x = 0; @@ -12,7 +11,7 @@ //// export as namespace bar; verify.importFixAtPosition([ -`import * as bar from "./foo"; +`import bar from "./foo"; export var x = 0; bar();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts index 72f6fcd2d1585..3421d603297b5 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports2.ts @@ -1,5 +1,4 @@ /// -// @Module: commonjs // @AllowSyntheticDefaultImports: false // @Filename: a/f1.ts @@ -12,7 +11,7 @@ //// export as namespace bar; verify.importFixAtPosition([ -`import bar = require("./foo"); +`import * as bar from "./foo"; export var x = 0; bar();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts b/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts deleted file mode 100644 index c6c2a5548741c..0000000000000 --- a/tests/cases/fourslash/importNameCodeFixNewImportAllowSyntheticDefaultImports3.ts +++ /dev/null @@ -1,18 +0,0 @@ -/// -// @Module: system - -// @Filename: a/f1.ts -//// [|export var x = 0; -//// bar/*0*/();|] - -// @Filename: a/foo.d.ts -//// declare function bar(): number; -//// export = bar; -//// export as namespace bar; - -verify.importFixAtPosition([ -`import bar from "./foo"; - -export var x = 0; -bar();` -]); \ No newline at end of file From 3800b46350de91cf9b51405218ef9e05c1e0b97a Mon Sep 17 00:00:00 2001 From: Aluan Haddad Date: Mon, 6 Nov 2017 19:07:52 -0500 Subject: [PATCH 6/6] remove errant whitespace --- src/compiler/checker.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index c937160ae7277..cb77ad85cebd6 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -64,7 +64,7 @@ namespace ts { const languageVersion = getEmitScriptTarget(compilerOptions); const modulekind = getEmitModuleKind(compilerOptions); const noUnusedIdentifiers = !!compilerOptions.noUnusedLocals || !!compilerOptions.noUnusedParameters; - const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); + const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); const strictNullChecks = getStrictOptionValue(compilerOptions, "strictNullChecks"); const strictFunctionTypes = getStrictOptionValue(compilerOptions, "strictFunctionTypes"); const noImplicitAny = getStrictOptionValue(compilerOptions, "noImplicitAny");