Use realpathSync.native on case-insensitive file systems by amcasey · Pull Request #44966 · microsoft/TypeScript (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
...chiefly, on Windows.
After re-reviewing all usages of realpath in the codebase, I believe there were three places where the input and output of realpath were being compared without specifically taking case into account. Fixing the two in module resolution as a little involved, since they didn't formerly have access to the case-sensitivity flag.
Fixes #43105
If we don't like the API change, I have another version that threads a parameter through all the places that need it.
I still need to do some manual testing around scenarios like this.
Edit: confirmed that main crashes with realpathSync.native re-enabled and this branch doesn't.
I regard this as fairly risky, so my vote would be to defer it until 4.5.
Heya @amcasey, I've started to run the tarball bundle task on this PR at 4c4dbdd. You can monitor the build here.
I marked this as a draft because I don't want it to get merged by accident, but it's ready for review.
Hey @amcasey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:
{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/106510/artifacts?artifactName=tgz&fileId=A03DF79ED70DC9EF30D6BCA883212716086BE27CCED7755706BAA3912589630C02&fileName=/typescript-4.4.0-insiders.20210709.tgz"
}
}
and then running npm install.
There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-44966-4".;
| @@ -280,6 +280,11 @@ namespace ts { |
|---|
| } |
| const nodeModulesAtTypes = combinePaths("node_modules", "@types"); |
| function arePathsEqual(path1: string, path2: string, host: ModuleResolutionHost): boolean { |
| const useCaseSensitiveFileNames = typeof host.useCaseSensitiveFileNames === "function" ? host.useCaseSensitiveFileNames() : host.useCaseSensitiveFileNames; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect the host to change its answer over time? Seems like you can avoid calling this over and over for every set of paths, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think it's ridiculous that some hosts expose it as a function, but I'm not sure how comfortable I am with ignoring that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose assuming that it won't change over time is no more outrageous than assuming that every implementation of realpath preserves lettercase when possible.
...so that path comparisons can use it during module resolution.
Force push is a rebase. Only notable merge conflict was undeleting the function Andrew mentioned above.
BobobUnicorn pushed a commit to BobobUnicorn/TypeScript that referenced this pull request
Make getSourceOfProjectReferenceRedirect take a Path
Add useCaseSensitiveFileNames to ModuleResolutionHost
...so that path comparisons can use it during module resolution.
- Re-enable realpathSync.native for case-insensitive file systems