8000 Code fix for missing imports by paulvanbrenk · Pull Request #11768 · microsoft/TypeScript · GitHub
[go: up one dir, main page]
Skip to content

Code fix for missing imports #11768

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Nov 17, 2016
Merged

Code fix for missing imports #11768

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0389207
Add codefix for missing imports + tests
Oct 20, 2016
eb46886
Re-order and cleanup
Oct 20, 2016
80d7f5c
refactor
zhengbli Nov 3, 2016
a6807ab
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
zhengbli Nov 3, 2016
1502f75
make tests pass
zhengbli Nov 3, 2016
e72f0c8
Make import specifier for new imports more comprehensive
zhengbli Nov 8, 2016
d273308
Fix existing import cases
zhengbli Nov 8, 2016
35d668a
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
zhengbli Nov 8, 2016
eb892d3
refactor
zhengbli Nov 9, 2016
6d024dc
Fix multiple import statement case
zhengbli Nov 10, 2016
26cd24a
add multiple code fixes and code action filtering and polishing
zhengbli Nov 11, 2016
a282fc0
not using the generic verify method for import fixes.
zhengbli Nov 11, 2016
1f82ce7
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
zhengbli Nov 11, 2016
d83c292
Correct insert position for new imports
zhengbli Nov 14, 2016
7568f6f
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
zhengbli Nov 16, 2016
aa2891c
improve the code action filtering logic
zhengbli Nov 16, 2016
055d20f
Fix line ending issue
zhengbli Nov 16, 2016
11ea6b3
cache where we can
zhengbli Nov 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor
  • Loading branch information
@zhengbli
zhengbli committed Nov 9, 2016
commit eb892d38a87a507ad009a0bd1f96eae38a8f63da
90 changes: 40 additions & 50 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ namespace ts.codefix {
}
}

const currentTokenMeaning = getMeaningFromLocation(token);
for (const moduleSymbol of allPotentialModules) {
context.cancellationToken.throwIfCancellationRequested();

Expand All @@ -28,8 +29,6 @@ namespace ts.codefix {
continue;
}

const currentTokenMeaning = getMeaningFromLocation(token);

// check the default export
const defaultExport = moduleExports["default"];
if (defaultExport) {
Expand Down Expand Up @@ -85,58 +84,30 @@ namespace ts.codefix {
}

function getCodeActionForExistingImport(declaration: ImportDeclaration | ImportEqualsDeclaration): CodeAction {
let namespacePrefix: string;
let moduleSpecifier: string;
if (declaration.kind === SyntaxKind.ImportDeclaration) {
const namedBindings = declaration.importClause && declaration.importClause.namedBindings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to handel the default import case, if you already have:

import Foo, { other } from "./b";
import Foo, * as ns from "./b";

if (namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport) {
namespacePrefix = (<NamespaceImport>namedBindings).name.getText();
if (namedBindings) {
if (namedBindings.kind === SyntaxKind.NamespaceImport) {
return getCodeActionForNamespaceImport(namedBindings.name.getText());
}
/**
* If the existing import declaration already has a named import list, just
* insert the identifier into that list.
*/
const textChange = getTextChangeForImportList(namedBindings);
return createCodeAction(
Diagnostics.Add_0_to_existing_import_declaration_from_1,
[name, declaration.moduleSpecifier.getText()],
textChange.newText,
textChange.span,
sourceFile.fileName
);
}

moduleSpecifier = declaration.moduleSpecifier.getText();
}
else {
namespacePrefix = declaration.name.getText();
moduleSpecifier = declaration.moduleReference.getText();
// insert a new import statement in a new lines
return getCodeActionForNewImport(declaration.moduleSpecifier.getText(), declaration.getEnd());
8000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is here. You already have an existing import for the module, why would you need to add another statement importing from the same module? Is this only if the existing statement contains a namespace import, and you want to add an import list import? (Is that valid/recommended?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is used when we want to add a new import list, while the existing one is a namespace import.

And in the latest implementation, when you have:

import * as ns from "module";
foo // foo is a member from ns

We provide two options:
1. change your code: so foo becomes ns.foo
2. add import list, so it becomes:

import * as ns from "module";
import { foo } from "module";
foo // foo is a member from ns

The user can choose from these two.

It is valid code, and as we cannot delete any user's code, I think these are the only options.

}

/**
* Cases:
* import * as ns from "mod"
* import default, * as ns from "mod"
* import ns = require("mod")
*
* Because there is no import list, we alter the reference to include the
* namespace instead of altering the import declaration. For example, "foo" would
* become "ns.foo"
*/
if (namespacePrefix) {
return createCodeAction(
Diagnostics.Change_0_to_1,
[name, `${namespacePrefix}.${name}`],
`${namespacePrefix}.`,
{ start: token.getStart(), length: 0 },
8000 sourceFile.fileName
);
}
/**
* If the existing import declaration already has a named import list, just
* insert the identifier into that list.
*/
else if (declaration.kind === SyntaxKind.ImportDeclaration &&
declaration.importClause &&
declaration.importClause.namedBindings &&
declaration.importClause.namedBindings.kind === SyntaxKind.NamedImports) {
const textChange = getTextChangeForImportList(declaration.importClause.namedBindings);
return createCodeAction(
Diagnostics.Add_0_to_existing_import_declaration_from_1,
[name, moduleSpecifier],
textChange.newText,
textChange.span,
sourceFile.fileName
);
}
return getCodeActionForNewImport(moduleSpecifier, declaration.getEnd());
return getCodeActionForNamespaceImport(declaration.name.getText());

function getTextChangeForImportList(importList: NamedImports): TextChange {
if (importList.elements.length === 0) {
Expand Down Expand Up @@ -174,6 +145,26 @@ namespace ts.codefix {
span: { start: insertPoint, length: 0 }
};
}

function getCodeActionForNamespaceImport(namespacePrefix: string): CodeAction {
/**
* Cases:
* import * as ns from "mod"
* import default, * as ns from "mod"
* import ns = require("mod")
*
* Because there is no import list, we alter the reference to include the
* namespace instead of altering the import declaration. For example, "foo" would
* become "ns.foo"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would rather we add a new import defalcation regardless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up providing two code actions to the user, and he/she can do either one. I think C# has the same behavior.

*/
return createCodeAction(
Diagnostics.Change_0_to_1,
[name, `${namespacePrefix}.${name}`],
`${namespacePrefix}.`,
{ start: token.getStart(), length: 0 },
sourceFile.fileName
);
}
}

function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): CodeAction {
Expand Down Expand Up @@ -260,7 +251,6 @@ namespace ts.codefix {
relativeName = removeFileExtension(relativeName);

if (options.paths) {
// TODO: handle longest match support
for (const key in options.paths) {
for (const pattern of options.paths[key]) {
const indexOfStar = pattern.indexOf("*");
Expand Down
0