Only add expected declaration files, avoid generated declarations #284
Conversation
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
==========================================
+ Coverage 58.83% 58.85% +0.01%
==========================================
Files 14 14
Lines 2128 2129 +1
Branches 351 350 -1
==========================================
+ Hits 1252 1253 +1
- Misses 725 726 +1
+ Partials 151 150 -1
Continue to review full report at Codecov.
|
src/project-manager.ts
Outdated
| const fileName = uri2path(uri); | ||
| if (isGlobalTSFile(fileName) || (!isDependencyFile(fileName) && isDeclarationFile(fileName))) { | ||
| if (isGlobalTSFile(fileName) || | ||
| (isDeclarationFile(fileName) && expectedFilePaths.includes(toUnixPath(fileName)))) { |
There was a problem hiding this comment.
This excludes non-declaration files in expected files.
There was a problem hiding this comment.
At first I though this reverted tomv564@021d1b4 but it still looks in this.fs.uris() 👍
- when adding basic files, search for them project-wide instead of searching in expected files list. The reason is that files may be excluded by tsconfig.json rule
src/project-manager.ts
Outdated
| } | ||
|
|
||
| // paths have been forward-slashified by TS (see toUnixPath below) | ||
| // TODO: consider moving expectedFilePaths out of LanguageServiceHost? |
There was a problem hiding this comment.
Funny, I always thought TS required expectedFilePath to be implemented. Turns out it doesn't and it's not even used inside the class. We can totally move it to ProjectConfiguration.
| /** | ||
| * List of files that project consist of (based on tsconfig includes/excludes and wildcards). | ||
| * Each item is a relative file path | ||
| */ |
There was a problem hiding this comment.
Make it private please and initialize here instead of in the constructor
|
Didn't fix #285 unfortunately :( |
|
Is it possible to write a unit test for this? |
Keep expectedFilePaths in a Set that is iterated separately.
src/project-manager.ts
Outdated
| } | ||
| } | ||
|
|
||
| // Add files that belong to the project according to tsconfig.json settings |
There was a problem hiding this comment.
This changes the previous behaviour of only adding declaration files project-wide and adding regular TS files as-needed. Now there is essentially nothing loaded on-demand anymore, are you sure this is what you want?
There was a problem hiding this comment.
You are correct, will revert
|
There is likely a bit of set-up effort for a unit test of The dependencies are also a bit tricky, e.g. I could look at adding these tests in a couple of days, but I'd love to hear what you think of ProjectConfiguration's testability. |
|
It would be possible to test the methods in ProjectConfiguration by stubbing all dependencies with sinon and asserting calls, but I don't think there would be much value to it, because it would be very easy to write tests that assert wrong behaviour. The way ensureBasicFiles etc work is a performance trade off (adding the minimal amount of files). I think it makes more sense to have tests that ensure specific hovers/j2d in specific workspaces still work, so we can change the implementation as long as these cases work. |
2 participants
As discussed in #208, TS will publish diagnostics when it receives empty content when importing a module.
Normally, the content is guaranteed to be in place for file
src/typescript-service.tsbyensureReferencedFiles, but TS was found to be requesting imports like 'vscode-jsonrpc' even when opening an import-free file like 'src/ast.ts'.The imports were traced back to compiler-emitted declarations like
lib/connection.d.tswhich were exposed to TS byensureBasicFiles.This PR reduces the adding of declarations to only those matching the project configuration for source files.
Besides fixing the repo in #208, this PR allows the tests in #261 to pass and should improve the startup speed of the service on projects with emitted declarations.