Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use fs.realpathSync.native when available #41292
Use fs.realpathSync.native when available #41292
Changes from 7 commits
4c44730
b5ef197
41e9014
70e99f9
f57ae39
2bccda7
b325dba
fca073b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think the testcase missing that we discussed and is important change is when current directory is "c:/temp" but on disk it is "C:/Temp" and all realpath results with new API will differ in casing and give errors vs it wasn't giving earlier ?
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.
And may be it is ok for watch scenario but it is something that will impact tsserver and in turn vscode.. vscode keeps track of open files from previous session so not sure if that gets updated if you change the casing and restart vscode from directory but users will not be able to get rid of error easily unless they close all files if the casing is retained across sessions by vscode. You would need to experiment to figure that out.
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 attempted to provide test coverage for drive root casing differences here. It's entirely possible I misunderstood your concerns though - please let me know if there's something else you'd like me to test.
From our offline discussion (before the break), I thought I understood that the watch VFS was the best place to add these tests. It sounds like you may be suggesting adding server tests as well. If that's the case, can you please point me at an example I can model them on?
I think you're saying that the paths returned by the server to VS Code might reflect the casing returned by realpathSync, but I'm not sure I understand what consequences you expect from this. If it's about re-opening files in a new session, I would have guessed VS Code used case-insensitive system calls to open them. If it's about VS Code passing stale paths back to the server when it starts a fresh instance, I would have guessed they would be re-normalized with an additional realpathSync call. Can you please elaborate on your concerns?
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.
That test case also gives incorrect result? This should have been earlier been error and should be error because on case sensitive file system the import will not resolve, which is what intention of
forceConsistentFileNames
is..The case that it doesnt verify is also when the files on disk and all imports are consistent, but you start with current directory = different casing and build the project. Previously that would not error now it will ?
If the above scenario breaks for watch there is a way to fix those errors by closing tsc and restarting from correct casing directory. But what happens in vscode is question. vscode keeps list of open files, so is the solution to close all files and open editor again? Does restart work , depending on answers to that we may need coverage for some tests here to lock the expected behaviour for baselines. There are tests for these in
unittests\tsserver\forceConsistentCasingInFileNames.ts
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 had the impression that
forceConsistentFileNames
ignored drive roots. Sorry for not including a comment to that effect.I'm not sure I follow. Where is the current directory specified in this case?
So you'd just like me to manually verify that VS Code doesn't have obvious breaks if I close a folder, change the casing on disk, and re-open it?
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.
Yes. But your import doesnt test import "somethingsomething/a" and "somethingsomething/A" you would get error on either one of those and that was expected behavior which is not the current case.
Current directory need to be specified as part of the https://github.com/microsoft/TypeScript/pull/41292/files#diff-742ed890d46f805d54cbcf33f4a6f2cd286a81ac5df890b0b20636445b464093R149 . We need test where current directory differs in casing from whats on the disk.
The bigger missing part is import in wrong casing should give error which it wont because the realpath will resolve to casing on the disk and then we will loose that info. But same code in case sensitive file system will give error because import will not be resolved.
Disk file name: a.ts
import "a" > success
import "A" > will fail on case sensistive file systems... so it should error with forceConsistentFileCasing on case insensitive filesystem which it would not when realpath is invoked as part of this change.