-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Conversation
Hi @riknoll, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
@paulvanbrenk please take a look when you have a moment |
@@ -2,6 +2,8 @@ | |||
|
|||
/* @internal */ | |||
namespace ts { | |||
const ambientModuleSymbolRegex = /^".+"$/; |
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.
I was copying the behavior where we access the modules elsewhere in the checker (see resolveExternalModuleNameWorker()
)
@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) => { |
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.
Use ts.forEach.. since this function won't work downlevel
@mhegazy or @RyanCavanaugh, mind taking a look? |
@@ -2808,6 +2826,15 @@ namespace ts { | |||
} | |||
} | |||
|
|||
function isInReferenceComment(sourceFile: SourceFile, position: number): boolean { |
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.
probably should move this to utilities.
result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName)); | ||
} | ||
} | ||
else if (host.getDirectories && options.typeRoots) { |
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.
get the effective typeRoots here, see getEffectiveTypeRoots
in program. we can export this function and use it here as well.
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; |
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.
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)
👍 after the 2 remaining comments |
we will need to merge this into release-2.0.5 as well as master. |
thanks. 👍 |
So is this in 1.5? How does it work? |
VSCode 1.5 ships with TS 1.8 in box. this is likely going to be in the October release of VSCode. |
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! |
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? |
I don't think it's in.. |
Fixes #188
This adds text completions in imports and triple slash references for the following items:
For triple slash references:
For imports and require:
@types
folder,tsconfig.json
, etc.)package.json
This is a breaking change that requires surfacing some of the
sys
object's file system reading APIs to theLanguageServiceHost
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.