Skip to content
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

Support getCompletions in external module name references in imports and exports #2173

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 64 additions & 0 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1374,6 +1374,8 @@ module ts {
static constElement = "const";

static letElement = "let";

static stringElement = "string";
}

export class ScriptElementKindModifier {
Expand Down Expand Up @@ -2483,6 +2485,11 @@ module ts {
log("getCompletionsAtPosition: Get previous token 2: " + (new Date().getTime() - start));
}

// Check of this is an external module name completion location
Copy link
Contributor

Choose a reason for hiding this comment

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

"Check if"

if (isExternalModuleName(previousToken)) {
return getExternalModulecompletions(previousToken);
}

// Check if this is a valid completion location
if (previousToken && isCompletionListBlocker(previousToken)) {
log("Returning an empty list because completion was requested in an invalid position.");
Expand Down Expand Up @@ -2912,6 +2919,63 @@ module ts {

return filteredMembers;
}

function isExternalModuleName(token: Node): boolean {
if (token && token.parent && token.kind === SyntaxKind.StringLiteral && isInStringOrRegularExpressionOrTemplateLiteral(token)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A few questions:

  1. Why check token.parent here, and not in the return expression? You don't seem to use parent here.
  2. Why check isInStringOrRegularExpressionOrTemplateLiteral given that you've already determined it's a StringLiteral?

Copy link
Member

Choose a reason for hiding this comment

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

He's checking if it's in the literal.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it is a literal. What does it mean for a literal to be in another literal? (Besides a literal nested in a template substitution expression, which is not interesting here).

Copy link
Member

Choose a reason for hiding this comment

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

isInStringOrRegularExpressionOrTemplateLiteral is a local helper function that captures the position of the cursor and determines if the cursor is inside of any of the implied literals.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is a case where token.kind === SyntaxKind.StringLiteral is true, but isInStringOrRegularExpressionOrTemplateLiteral(token) is false?

Copy link
Member

Choose a reason for hiding this comment

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

When token is a terminated string literal but the cursor position resides beyond the end position of the token.

return token.parent.kind === SyntaxKind.ImportDeclaration ||
token.parent.kind === SyntaxKind.ExportDeclaration ||
token.parent.kind === SyntaxKind.ExternalModuleReference;
Copy link
Contributor

Choose a reason for hiding this comment

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

Be careful not to include:

export = "hello";

if we've decided to support that. Also, don't include:

export default "hello";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. will fix it.

}
return false;
}

function getExternalModulecompletions(location: Node): CompletionInfo {
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize the 'c'

Copy link
Member

Choose a reason for hiding this comment

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

Leave a comment about what kinds of completions we support, then leave a small example. For instance, we don't hit the disk to look up available files (and note this in the comment), so be explicit, since I find that modules are the most arcane part of the compiler and language.

var entries: CompletionEntry[] = [];
var currentSourceFile = location.getSourceFile();
var currentDirectoryPath = getDirectoryPath(normalizePath(currentSourceFile.fileName));
var isInAmbientContext = ts.isInAmbientContext(location);

forEach(program.getSourceFiles(), sourceFile => {
if (isExternalModule(sourceFile)) {
// Only consider these if current request is not in an ambient context,
// and in a diffrent file
if (!isInAmbientContext && sourceFile !== currentSourceFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not for ambients? You can import a nonambient module in an ambient one, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah.. but chances are you have all your dependencies in ambients, and you will reference them but not the opposite. it just to limit clutter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, just wanted to make sure we considered it.

var relativeModuleName = removeFileExtension(getRelativePathToDirectoryOrUrl(currentDirectoryPath,
Copy link
Member

Choose a reason for hiding this comment

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

Separate this into two separate calls - store the intermediate result in a variable called relativeFilePath or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

Because it's currently spanning two lines and hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not convinced it is hard to read :)

sourceFile.fileName, "", getCanonicalFileName, /*isAbsolutePathAnUrl*/ true));
Copy link
Contributor

Choose a reason for hiding this comment

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

What's isAbsolutePathAnUrl?

if (relativeModuleName.charCodeAt(0) !== CharacterCodes.dot && relativeModuleName.charCodeAt(1) !== CharacterCodes.slash) {
Copy link
Member

Choose a reason for hiding this comment

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

Check whether or not you have 2 characters to use.

Copy link
Member

Choose a reason for hiding this comment

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

Also add a line above the if and move the comment to above the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will get undefined, which us fine as it passes the test.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose; if it deoptimizes, it may not be as critical as the batch compiler. Still not a painful check though.

// always make files relative
relativeModuleName = "./" + relativeModuleName;
}
entries.push({
name: relativeModuleName,
kind: ScriptElementKind.stringElement,
kindModifiers: ScriptElementKindModifier.none,
});
}
}
else {
// Look for ambient external module declarations
forEachChild(sourceFile, child => {
if (child.kind === SyntaxKind.ModuleDeclaration && (<ModuleDeclaration>child).name.kind === SyntaxKind.StringLiteral) {
if (sourceFile !== currentSourceFile || location.pos > child.end || location.end < child.pos) {
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure there is a helper function to see if something intersects with a node, and you can invert that result. Consider using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is interesting. I guess the reason you don't want currentSourceFile is because a file is not allowed to simultaneously contain an ambient external module and an import.

entries.push({
kind: ScriptElementKind.stringElement,
kindModifiers: ScriptElementKindModifier.ambientModifier,
name: (<ModuleDeclaration>child).name.text
});
}
}
});
}
});

return {
isMemberCompletion: true,
isNewIdentifierLocation: true,
isBuilder: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't understand why these are true. It certainly doesn't seem like a member completion. Also, what is the difference between isNewIdentifierLocation and isBuilder? I thought those both correspond to the notion that an entry should not be implicitly committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isBuilder needs to be removed. now it is just an alias for isNewIdentifierLocation.

i do not think isMemberCompletion is used by the editors except VS2012. so we should be removing this as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, so isNewIdentifierLocation is true because the user might type a module reference that we are not statically aware of?

entries
};
}
}

function getCompletionEntryDetails(fileName: string, position: number, entryName: string): CompletionEntryDetails {
Expand Down
29 changes: 29 additions & 0 deletions tests/cases/fourslash/externalModuleNameCompletions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/// <reference path='fourslash.ts'/>

// @Filename: ambients.ts
////declare module "a" {}
////declare module "b" {}

// @Filename: moduleC.ts
////export var x = 0;

// @Filename: global.ts
////var global = 0;

// @Filename: current.ts
////import * from "/*1*/";
////import d from "/*2*/";
////import {a as A} from "/*3*/";
////import * as NS from "/*4*/";
////import "/*5*/";
////import x = require("/*6*/");
////export * from "/*7*/";
////export {a as A} from "/*8*/";

test.markers().forEach(m => {
goTo.position(m.position, m.fileName);
verify.memberListContains("a");
verify.memberListContains("b");
verify.memberListContains("./moduleC");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we always want "./moduleC" and not "moduleC"? Does this work correctly with node_modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It felt more consistent, since the other ones in different directories "..", just to separate them from ambient.. but i can change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have that much experience with using modules, but I get the impression that the plain name is more common than the relative path. I admit I could be wrong though.

verify.memberListCount(3);
});
22 changes: 22 additions & 0 deletions tests/cases/fourslash/externalModuleNameCompletions2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/// <reference path='fourslash.ts'/>

// @Filename: ambients.ts
////declare module "a" {}
////declare module "b" {}

// @Filename: moduleC.ts
////export var x = 0;

// @Filename: global.ts
////var global = 0;

// @Filename: current.ts
////declare module "d" {
//// import * from "/*1*/";
////}

goTo.marker("1");
verify.memberListContains("a");
verify.memberListContains("b");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we not want moduleC to appear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well.. just cause this is ambient, so you want it to depend on other ambient things. more of limiting the clutter in .d.ts completion list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, this ties in with my question from the source code.

verify.memberListCount(2);

24 changes: 24 additions & 0 deletions tests/cases/fourslash/externalModuleNameCompletions3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// <reference path='fourslash.ts'/>

// @Filename: moduleA.ts
////export var x = 0;

// @Filename: folderA/moduleA.ts
////export var x = 0;

// @Filename: folderA/folderAA/moduleAA.ts
////export var x = 0;

// @Filename: folderA/folderAA/folderAAA/moduleAAA.ts
////export var x = 0;

// @Filename: folderA/folderAA/folderAAB/current.ts
////import * from "/*1*/";

goTo.marker("1");
verify.memberListContains("../folderAAA/moduleAAA");
verify.memberListContains("../moduleAA");
verify.memberListContains("../../moduleA");
verify.memberListContains("../../../moduleA");
verify.memberListCount(4);