(fix) case insenstive file system document sync #1697
Conversation
…nto case-insenstive-doc-sync
|
Keeping the original is so that the hover info of a module path would show the original casing. And also that the URI converted from the typescript response is the original casing. It probably only happens in cross-document features so it probably only in typescript features. It's mostly the non-project files that would have the difference. Because the casing typescript uses is from the About the multiple, originally I was thinking it might be more compatible with the old code. But maybe that is just hiding potential bugs. We should convert the casing before comparing. After writing these. I think I can remove the original casing from the |
to make I didn't break it later
|
|
||
| const workspaceEdit = await updateImportsProvider.updateImports({ | ||
| // imported files both old and new have to actually exist, so we just use some other test files | ||
| oldUri: pathToUrl(join(testFilesDir, 'diagnostics', 'diagnostics.svelte')), | ||
| oldUri: pathToUrl(join(testFilesDir, 'imported.svelte')), |
There was a problem hiding this comment.
This comment seems to be outdated. It now seems to be able to check even if the new file does not exist.
|
I have scanned through the codebase for the |
| ) { | ||
| this.onSnapshotChange = this.onSnapshotChange.bind(this); | ||
| this.globalSnapshotsManager.onChange(this.onSnapshotChange); | ||
| this.documents = new FileMap(useCaseSensitiveFileNames); | ||
| this.projectFileToOriginalCasing = new FileMap(useCaseSensitiveFileNames); |
There was a problem hiding this comment.
Not quite understand the use case if this map here - it's not set with original,case_insensitive or similar, it's set with originalFileName, originalFileName in all places where it's called
There was a problem hiding this comment.
It's essentially Map<case_insensitive, original>. I think I can change it to directly use Map<case_insensitive, original> so it's more clear on its use case.
There was a problem hiding this comment.
What I meant is that everywhere where set is invoked you call it with the same variable for both parameters
There was a problem hiding this comment.
Yes. Because the FileMap will transform the casing for the key but not the value. That is indeed confusing as it keeps two different things.
| ...snapshotManager | ||
| .getFileNames() | ||
| .filter( | ||
| (file) => !canonicalProjectFileNames.includes(getCanonicalFileName(file)) |
There was a problem hiding this comment.
I believe this method is called rather often, and this would make it O(n^2) - can we use a Set instead?
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
to be more clear on the intention
#1627 Adopted the casing conversion helper function from typescript. And added two collections for tracking files case intensively in case-insensitive file systems. Another goal is to also address the famous "files differ only in casing" typescript error when it's VSCode using the old casing before renaming.
The strategy is that we store the lowercase filenames in the collection. And when retrieving or checking the existence of files. We convert it to lowercase before checking. And we keep the original casing in another place so we can preserve the casing. Just not sure if we need to keep track of multiple. Not sure if we should reach the filesystem to check the correct casing if there're multiple exists. Because the only way I can think of is to use the read directory of the parent directory. Not sure if it's worth the effort.
Created as a draft to discuss if the overall strategy has any flaws. Also need to scan through the codebase to check for any filename equality check. We might also need to normalize the casing before checking.