Skip to content
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 realpathSync.native on case-insensitive file systems #44966

Merged
merged 3 commits into from
Aug 26, 2021

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Jul 9, 2021

...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

@amcasey amcasey requested review from orta and andrewbranch July 9, 2021 23:08
@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jul 9, 2021
@amcasey
Copy link
Member Author

amcasey commented Jul 9, 2021

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.

@amcasey
Copy link
Member Author

amcasey commented Jul 9, 2021

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 9, 2021

Heya @amcasey, I've started to run the tarball bundle task on this PR at 4c4dbdd. You can monitor the build here.

@amcasey
Copy link
Member Author

amcasey commented Jul 9, 2021

I marked this as a draft because I don't want it to get merged by accident, but it's ready for review.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 9, 2021

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;
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jul 16, 2021

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@amcasey amcasey marked this pull request as ready for review August 16, 2021 21:40
@amcasey
Copy link
Member Author

amcasey commented Aug 26, 2021

Force push is a rebase. Only notable merge conflict was undeleting the function Andrew mentioned above.

@amcasey amcasey merged commit 7fc1cb4 into microsoft:main Aug 26, 2021
@amcasey amcasey deleted the RealpathWin branch August 26, 2021 22:35
BobobUnicorn pushed a commit to BobobUnicorn/TypeScript that referenced this pull request Oct 24, 2021
…4966)

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

"Cannot read property 'lastIndexOf' of undefined" error in tsserver.js
5 participants