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.

Sign up for GitHub

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

Jump to bottom

fix(typescript): Drive letter casing on win32 platforms #1134

Merged
merged 1 commit into from
Apr 13, 2022
Merged

fix(typescript): Drive letter casing on win32 platforms #1134

merged 1 commit into from
Apr 13, 2022
+1 −1

Conversation

lovettchris
Copy link
Contributor

@lovettchris lovettchris commented Mar 11, 2022 •
edited
Loading

Rollup Plugin Name: plugin-typescript

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

As in several other places in this code base the regex for matching windows drive letters should be /^([a-zA-Z]+):/ but one case was missing in the implementation of TSCache.

@lovettchris lovettchris requested a review from shellscape as a code owner March 11, 2022 03:36
@shellscape
Copy link
Collaborator

Thanks for the PR. I see you checked the box for tests included, but don't see any tests in your PR. (Nearly) All fixes and features require an accompanying test to be accepted, even if the change looks obvious. It's required to protect against regression.

@lovettchris lovettchris changed the title fix github issue 1133 fix: github issue 1133 Mar 11, 2022
@lovettchris lovettchris changed the title fix: github issue 1133 fix(typescript): github issue 1133 Mar 11, 2022
@lovettchris lovettchris changed the title fix(typescript): github issue 1133 fix(plugin-typescript): github issue 1133 Mar 11, 2022
@lovettchris
Copy link
Contributor Author

@shellscape, the existing tests failed on my machine and with my fixes they pass. The problem is in figuring out how to automate the machine setup. One needs a windows drive installed in such way that node.js fs.realpath returns a different case from the path.resolve function and I haven't figured out how or why my machine is doing that. I supposed one could implement some kind of fs hook during testing that randomizes file name case on win32 platforms... and this might catch more bugs...

@lovettchris lovettchris changed the title fix(plugin-typescript): github issue 1133 fix(plugin-typescript): Drive letter bug on win32 platforms Mar 11, 2022
@lovettchris
Copy link
Contributor Author

Can you suggest the right title for this PR, I've tried a million variations they all failed...

@shellscape shellscape changed the title fix(plugin-typescript): Drive letter bug on win32 platforms fix(typescript): Drive letter casing on win32 platforms Mar 11, 2022
@shellscape
Copy link
Collaborator

(Title taken care of. It didn't like the hyphen in the scope)

OK Thanks for the explanation on the test. That's pretty much all we need to archive to let that slide. Anyone wondering why it has no test can point back to your comment. WRT drives on Windows, it's not exactly easy to create new mounts. Your best bet would be creating a virtual drive/ramdisk, but I went through that process for https://github.com/shellscape/webpack-plugin-ramdisk#windows-users and came up short. I'm not aware of any new capabilities in the newer versions of Windows either. You're welcome to try, but we'll give it a pass if you'd rather not.

@lovettchris
Copy link
Contributor Author

didn't like the hyphen in the scope

thanks for fixing the title. I will pass on figuring out how to test this for now since I'm just trying to unblock another project that is consuming all my time, but I could add a new work item in case someone else has time to test properly all windows file system case insensitivities...

@lovettchris
Copy link
Contributor Author

lovettchris commented Mar 11, 2022 •
edited
Loading

@shellscape it might be relatively easy to reproduce the bug on any windows machine. Create a shortcut that launches cmd.exe with the Start in path where the drive letter case is opposite of what you get from nodejs fs.realpath in that folder. In my case the opposite is lowercase, so this:
image

Then try and run the rollup tests with npm run ci:test:only and they will all fail like this:

image

@lovettchris lovettchris mentioned this pull request Mar 11, 2022
Closed
9 tasks
@shellscape shellscape mentioned this pull request Apr 13, 2022
Closed
9 tasks
@shellscape shellscape merged commit 1713bc0 into rollup:master Apr 13, 2022
shellscape pushed a commit that referenced this pull request Apr 13, 2022
@lovettchris @shellscape
fix(typescript): Drive letter casing on win32 platforms. fixes #1133 (#…
8d9dc0e 
…1134)
@lovettchris lovettchris deleted the clovett/fix_win32_drive_letter_case_insensitivity branch April 14, 2022 07:23
@lovettchris
Copy link
Contributor Author

lovettchris commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shellscape shellscape shellscape approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lovettchris @shellscape