-
Notifications
You must be signed in to change notification settings - Fork 12.9k
fix inconsistencies in import UMD code fixes adapting to module format #19572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
dcbadcb
3041e10
1be8b21
f27148d
78ee07f
3800b46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<AnyImportSyntax>): 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); | ||
} | ||
|
@@ -627,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; | ||
|
@@ -644,6 +665,20 @@ namespace ts.codefix { | |
Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here"); | ||
} | ||
|
||
const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); | ||
|
||
// Prefer to import as a synthetic `default` if available. | ||
if (allowSyntheticDefaultImports) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weswigham this is another place for your new change in #19675 to be plugged in. we would like to always show the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger. Thank you! |
||
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. | ||
return getCodeActionForImport(symbol, { ...context, symbolName, kind: ImportKind.Namespace }); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/// <reference path="fourslash.ts" /> | ||
// @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();` | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/// <reference path="fourslash.ts" /> | ||
// @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();` | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/// <reference path="fourslash.ts" /> | ||
// @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();` | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/// <reference path="fourslash.ts" /> | ||
// @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();` | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will never be the parent of a
LiteralExpression
. Removing in #19667.