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

Fix 188: Autocomplete for imports and triple slash reference paths #9353

Merged
merged 44 commits into from Sep 6, 2016

Conversation

riknoll
Copy link
Member

@riknoll riknoll commented Jun 24, 2016

Fixes #188
This adds text completions in imports and triple slash references for the following items:

For triple slash references:

  • Relative and absolute paths

For imports and require:

  • Relative and absolute paths
  • Modules discovered by the type checker (via triple slash references, @types folder, tsconfig.json, etc.)
  • Modules listed in your project's package.json

This is a breaking change that requires surfacing some of the sys object's file system reading APIs to the LanguageServiceHost

Note that this will only pick up node modules that are in package.json. This was done to improve efficiency and to make sure that sub-dependencies don't crowd the completion list.

@riknoll riknoll added the Breaking Change Would introduce errors in existing code label Jun 24, 2016
@msftclas
Copy link

Hi @riknoll, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Richard Knoll). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@riknoll
Copy link
Member Author

riknoll commented Jun 24, 2016

@paulvanbrenk please take a look when you have a moment

@@ -2,6 +2,8 @@

/* @internal */
namespace ts {
const ambientModuleSymbolRegex = /^".+"$/;
Copy link
Member Author

Choose a reason for hiding this comment

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

I was copying the behavior where we access the modules elsewhere in the checker (see resolveExternalModuleNameWorker())

@riknoll
Copy link
Member Author

riknoll commented Jun 29, 2016

@vladima would you mind taking a look at this when you have a chance?

@@ -135,7 +135,8 @@ namespace Harness.LanguageService {

public getFilenames(): string[] {
const fileNames: string[] = [];
ts.forEachValue(this.fileNameToScript, (scriptInfo) => {
this.virtualFileSystem.getAllFileEntries().forEach((virtualEntry) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ts.forEach.. since this function won't work downlevel

@riknoll
Copy link
Member Author

riknoll commented Jul 13, 2016

@mhegazy or @RyanCavanaugh, mind taking a look?

@@ -2808,6 +2826,15 @@ namespace ts {
}
}

function isInReferenceComment(sourceFile: SourceFile, position: number): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should move this to utilities.

result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName));
}
}
else if (host.getDirectories && options.typeRoots) {
Copy link
Contributor

Choose a reason for hiding this comment

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

get the effective typeRoots here, see getEffectiveTypeRoots in program. we can export this function and use it here as well.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 26, 2016

no more comments on my side. i would like to get @dbaeumer's feedback on the design approach though.

result.push(entry);
const { name, kind, kindModifiers, sortText, replacementSpan } = entry;

let convertedSpan: protocol.TextSpan = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we limit the creation of the new object to only if the replacement span exists. this avoids creating garbage in the common case (non-module name completion)

@mhegazy
Copy link
Contributor

mhegazy commented Sep 2, 2016

👍 after the 2 remaining comments

@mhegazy
Copy link
Contributor

mhegazy commented Sep 2, 2016

we will need to merge this into release-2.0.5 as well as master.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 3, 2016

thanks. 👍

@fxck
Copy link

fxck commented Sep 12, 2016

So is this in 1.5? How does it work?

@mhegazy
Copy link
Contributor

mhegazy commented Sep 12, 2016

VSCode 1.5 ships with TS 1.8 in box. this is likely going to be in the October release of VSCode.

@jbrownson
Copy link

I had been using Atom for TS dev for awhile, experimented w/ Code, seems better in every way except this, it's so nice to have this back and better than it was in Atom. Thanks!

@JohannesRudolph
Copy link

I don't quite get whether this is in VSCode 1.6 (which ships with TS 2.0.3)? I would assume that they'd mentioned such a massive improvement in their release announcements... or did this PR only add the "backend" support to the TS compiler?

@fxck
Copy link

fxck commented Oct 12, 2016

I don't think it's in..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants