GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Use realpathSync.native on case-insensitive file systems by amcasey · Pull Request #44966 · microsoft/TypeScript

Conversation

@amcasey

@amcasey Andrew Casey (amcasey) commented Jul 9, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@amcasey
Andrew Casey (amcasey) requested review from Andrew Branch (andrewbranch) and Orta Therox (orta) July 9, 2021 23:08
@typescript-bot TypeScript Bot (typescript-bot) assigned Andrew Casey (amcasey) Jul 9, 2021
@typescript-bot TypeScript Bot (typescript-bot) added the Author: Team label Jul 9, 2021
@typescript-bot TypeScript Bot (typescript-bot) assigned Orta Therox (orta) Jul 9, 2021
@typescript-bot TypeScript Bot (typescript-bot) added the For Milestone Bug PRs that fix a bug with a specific milestone label Jul 9, 2021
@amcasey

Andrew Casey (amcasey) commented Jul 9, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented Jul 9, 2021

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) pack this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 9, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented Jul 9, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Jul 9, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hey Andrew Casey (@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/[email protected]".;
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@weswigham Wesley Wigham (weswigham) mentioned this pull request Jul 12, 2021
Watch mode watches for changes in package.json files used in resolution #44935
Merged
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Jul 16, 2021
View reviewed changes
Comment thread src/compiler/moduleNameResolver.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;

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Jul 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@amcasey Andrew Casey (amcasey) Jul 16, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@amcasey Andrew Casey (amcasey) Jul 16, 2021

Copy link
Copy Markdown
Member Author

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 suppose assuming that it won't change over time is no more outrageous than assuming that every implementation of realpath preserves lettercase when possible.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@amcasey
Andrew Casey (amcasey) marked this pull request as ready for review August 16, 2021 21:40
andrewbranch
Andrew Branch (andrewbranch) reviewed Aug 25, 2021
View reviewed changes
Comment thread src/compiler/moduleNameResolver.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
andrewbranch
Andrew Branch (andrewbranch) approved these changes Aug 25, 2021
View reviewed changes
Andrew Casey (amcasey) added 3 commits August 26, 2021 10:50
@amcasey
Make getSourceOfProjectReferenceRedirect take a Path
0b345da
@amcasey
Add useCaseSensitiveFileNames to ModuleResolutionHost
6ce7989 
...so that path comparisons can use it during module resolution.
@amcasey
Re-enable realpathSync.native for case-insensitive file systems
9df6f71
@amcasey
Andrew Casey (amcasey) force-pushed the RealpathWin branch from 4c4dbdd to 9df6f71 Compare August 26, 2021 20:15
@amcasey

Andrew Casey (amcasey) commented Aug 26, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Force push is a rebase. Only notable merge conflict was undeleting the function Andrew mentioned above.
andrewbranch reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey
Andrew Casey (amcasey) merged commit 7fc1cb4 into microsoft:main Aug 26, 2021
@amcasey
Andrew Casey (amcasey) deleted the RealpathWin branch August 26, 2021 22:35
Cassandra Choi (BobobUnicorn) pushed a commit to BobobUnicorn/TypeScript that referenced this pull request Oct 24, 2021
@amcasey @BobobUnicorn
Use realpathSync.native on case-insensitive file systems (microsoft#4…
56fdf38 
…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
@sambacha sam bacha (sambacha) mentioned this pull request Mar 23, 2022
perf(fs): use realpathSync native for >15%. improvement [HH-446] NomicFoundation/hardhat#2522
Closed
1 of 3 tasks
@sheetalkamat Sheetal Nandi (sheetalkamat) mentioned this pull request Jun 10, 2022
forceConsistentCasingInFileNames doesn't work for modules in node_modules #49470
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser left review comments
@andrewbranch Andrew Branch (andrewbranch) andrewbranch approved these changes
@orta Orta Therox (orta) Awaiting requested review from orta

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@orta Orta Therox (orta)

@amcasey Andrew Casey (amcasey)

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@amcasey @typescript-bot @DanielRosenwasser @andrewbranch @orta
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.