fix: correct case sensitivity in core and e2e tests#2321
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
| // TODO[typescript>=6.0]: Remove this declaration. | ||
| // https://github.com/microsoft/TypeScript/pull/63046 |
There was a problem hiding this comment.
I don't see why we shouldn't upgrade to the TS 6.0 beta, but we can do that in a different PR.
packages/e2e/tests/utils.ts
Outdated
| return stdout | ||
| .replace(/\\/g, "/") | ||
| .replace(new RegExp(RegExp.escape(normalizedCwd), "gi"), "<cwd>"); |
There was a problem hiding this comment.
On macOS, /Users is capitalized, while the cwd our host uses isn't. This probably affected Windows as well.
CC @auvred; this continues to convince me we should preserve case in the VFS.
There was a problem hiding this comment.
I'm still getting this, I'm not sure why... @barrymichaeldoyle do you know what would've changed?
- <underline><cwd>/fixtures/CONTRIBUTING.md</underline>
+ <underline><cwd>/fixtures/contributing.md</underline>There was a problem hiding this comment.
Also don't know if we should switch to normalizePath as Auvred suggested but I'll leave it be for now.
There was a problem hiding this comment.
On macOS, /Users is capitalized, while the cwd our host uses isn't. This probably affected Windows as well.
That's why we should use normalizePath!
CC @auvred; this continues to convince me we should preserve case in the VFS.
This would be semantically incorrect. As I said earlier, fs.readFile('fIlE') === fs.readFile('file') on case insensitive file systems. VFS should match this behavior to avoid inconsistencies.
There was a problem hiding this comment.
Oh, I didn't see you replied, sorry. Got lost in the noise.
That's why we should use
normalizePath!
I agree that it's it's nice to use one implementation, but can you explain to my feeble brain how normalizePath helps here? Is it that it'd be lowercase everywhere?
As I said earlier,
fs.readFile('fIlE')===fs.readFile('file')on case insensitive file systems. VFS should match this behavior to avoid inconsistencies.
Yes, right, I agree. What I don't understand is why we can't normalize on operations instead of in our storage. It's nice to users to be able to keep /Users or C:/ capitalized. I, personally, would be (am) annoyed if you try to lowercase my paths, because they look different and I'm used to a certain case. Windows a little unusual because path normalization anyway, but, on macOS, it just gives weird vibes.
Ok, that was a longish rant, sorry. I'm sure there's a good reason for doing it this way, and I'm happy to concede, I just don't like it yet. So please enlighten me!
There was a problem hiding this comment.
they're suggesting not carrying around two paths, but only normalize at the point where you're using it (e.g. comparisons).
Ahh, I see, sorry! Well, this might actually work, but then we would need to adjust the paths returned from LinterHost methods to be non-normalized, I guess, for example from watchDirectory...
There was a problem hiding this comment.
I was hoping we could leave this for a followup, but the issue is actually that we do normalize the path here (I was hoping that normalizing everywhere would help).
I'll see what I can do :)
There was a problem hiding this comment.
Gave up my approach and tossed Amp on it, and well, there's a patch now, that seems to work in my testing. We do have two paths now, which I'm not thrilled with, but honestly my alternative was handrolling a hashmap with a custom equality so I'm not complaining. 😅
There was a problem hiding this comment.
Haha, sorry about the late reply. But yeah I don't remember the nitty gritties of this. So I don't have strong opinions on changes here.
1091bcd to
be836c3
Compare
653279a to
60dc7df
Compare
|
Ooops. Sorry for the rebase. |
60dc7df to
b23690a
Compare
Amp-Thread-ID: https://ampcode.com/threads/T-019c8802-4a92-719e-ad17-2a4c539dcce4 Co-authored-by: Amp <amp@ampcode.com>
c312723 to
59d6e01
Compare
@lishaduck Sorry for the late response. I don’t think it’s working at the moment. It might be because |
Just confirming: this PR does more than just e2e tests, it actually fixes case sensitivity things right? And fixes #2367? |
Right. It was originally fixing the tests' case-sensitivity but then I realized the actual case-sensitivity was broken so that all got rolled into this. |
6e4b900 to
6bc68ec
Compare
auvred
left a comment
There was a problem hiding this comment.
I like the "path key" terminology! Much clearer than TypeScript's file name and path. Left a couple minor questions, nothing critical (sorry it took me so long to review)
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Hell yeah! Nothing from me beyond what Auvred's already said in review. This is looking great!
Co-authored-by: auvred <61150013+auvred@users.noreply.github.com> Also a bit of Amp to fix what that broke: Amp-Thread-ID: https://ampcode.com/threads/T-019cdaf0-487a-7656-9652-8d879c64f373 Co-authored-by: Amp amp@ampcode.com
876233e to
1b16cac
Compare
|
I'm going to merge given how much this fixes. |
PR Checklist
status: accepting prsOverview
❤️