Fix global type loading on Windows paths #362
Conversation
Codecov Report
@@ Coverage Diff @@
## master #362 +/- ##
==========================================
- Coverage 74.68% 74.63% -0.06%
==========================================
Files 15 15
Lines 1758 1758
Branches 331 331
==========================================
- Hits 1313 1312 -1
- Misses 294 295 +1
Partials 151 151
Continue to review full report at Codecov.
|
…orward slashes Fixes typeroots support in Windows
|
@felixfbecker would you want to have a look at this? Not a clean diff, but I hope the comments above help explain. The main design issue I tried to solve is knowing when windows paths are converted to unix paths, my guidelines for solving the inconsistency were:
|
src/project-manager.ts
Outdated
| while (dir && dir !== rootPath) { | ||
| config = configs.get(dir) | ||
| if (config) { | ||
| return config | ||
| } | ||
| const pos = dir.lastIndexOf('/') | ||
| const pos = dir.includes('\\') ? dir.lastIndexOf('\\') : dir.lastIndexOf('/') |
There was a problem hiding this comment.
I'd use string.search()
There was a problem hiding this comment.
Definitely more compact:
filePath.search(/[\\\/][^\\\/]*$/)Do you prefer this? If so, I propose storing the regex as a static member somewhere (or adding this functionality to util.ts.
src/test/project-manager.test.ts
Outdated
| import { MapFileSystem } from './fs-helpers' | ||
| chai.use(chaiAsPromised) | ||
| const assert = chai.assert | ||
|
|
||
| describe('ProjectManager', () => { | ||
| for (const rootUri of ['file:///', 'file:///c:/foo/bar/', 'file:///foo/bar/']) { | ||
| testWithRootUri(rootUri) |
There was a problem hiding this comment.
why not inline this here?
There was a problem hiding this comment.
Fixed (if I understood correctly)
Could you make this documentation available in the source code somewhere, e.g. in the docblock of the respective classes? |
|
For visibility I propose adding it on every docblock that describes a field/parameter. I wish typing could help with this, eg type UnixPath: stringAn option is to use |
|
@felixfbecker your suggestions were applied, ready for your review again! |
src/project-manager.ts
Outdated
| @@ -24,6 +24,8 @@ import { | |||
| uri2path | |||
| } from './util' | |||
|
|
|||
| const LastForwardOrBackwardSlash = /[\\\/][^\\\/]*$/ | |||
This PR now contains 3 fixes for Windows path handling caused by two inconsistencies
The first inconsistency existed between
expectedFiles(unix paths) andtypeRoots(windows paths)Typings from e.g. jasmine/mocha not resolved by Typescript
Repro: Open any test file in this project, receive diagnostics about not finding
itanddescribeCause: Files were loaded in memory, but
ensureBasicFilespassed windows paths into unix-style path filters (isGlobalTsFile).Fix: Convert to unix paths before passing to filter methods in
ensureBasicFiles(7f57e21)Test: ✅ Added in 6d9d3d0
After this fix,
isExpectedDeclarationFilenow expected a unixPath, causing a new issue:Typings from typeRoots not resolved by Typescript
Repro: Open a project using typeRoots, receive diagnostics about not finding types from typeRoots
Cause: Files from typeRoots were not loaded into memory by
ensureConfigDependenciesas it was still passing Windows paths into the now unix-path-basedisConfigDependencyFix: Make
typeRootsunix-path based so it is consistent with checks againstexpectedFiles. (f0c8e02)Test: ✅ Updated to check host in 6d9d3d0
This fixed the type resolution issues I had been seeing while working on Windows. 🎉
To add test coverage, I made the ProjectManager tests use different rootUris. This did not cover the above issues, but it did uncover another inconsistency:
Windows
rootPathnot handled correctly for configuration lookups in ProjectManagerWhen ProjectManager is constructed with a Windows
rootPath:trimmedRootPath)this.configswhich otherwise uses unix paths.Impact: My guess is default configurations did not work on windows, as they were windows-style and the rest of the code assumed unix-style paths were used..
Fix: Handle
rootPathand configuration lookups in a platform-sensitive way (because there is no need to normalise to unix paths here?)