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

Fix case renames not marking previous casing as missing in time entries #143

Merged
merged 5 commits into from Feb 19, 2020

Conversation

ijjk
Copy link
Contributor

@ijjk ijjk commented Feb 15, 2020

As noticed in vercel/next.js#10351 when a filename's casing is changed both variants of the filename are reported from getTimeInfoEntries. This makes users unable rely on getTimeInfoEntries as a safe source of truth on case insensitive file systems.

This PR attempts to correct this by when an event is triggered for a file that is already in the watcher's files it re-checks that the file is still present with exact casing by doing a readdir on the directory the file resides in.

The added test cases were tested and previously failing on:

  • MacOS 10.14.6 (node: v12.14.1)
  • Windows 10 Pro 1809 (node: v10.15.3)

I also tested against the added test case in vercel/next.js#10351 to double check this has the expected behavior.

@codecov
Copy link

codecov bot commented Feb 15, 2020

Codecov Report

Merging #143 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #143      +/-   ##
=========================================
- Coverage   91.34%   91.2%   -0.15%     
=========================================
  Files           4       4              
  Lines         705     716      +11     
  Branches      185     188       +3     
=========================================
+ Hits          644     653       +9     
- Misses         61      63       +2
Impacted Files Coverage Δ
lib/DirectoryWatcher.js 89.45% <100%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24ad42f...ba80795. Read the comment docs.

lib/DirectoryWatcher.js Outdated Show resolved Hide resolved
lib/DirectoryWatcher.js Outdated Show resolved Hide resolved
@sokra sokra merged commit 1ba785d into webpack:master Feb 19, 2020
@sokra
Copy link
Member

sokra commented Feb 19, 2020

Thanks

@sokra
Copy link
Member

sokra commented Feb 19, 2020

If you want to you can also backport this to the watchpack-1 branch.

@ijjk ijjk deleted the fix/case-renaming branch February 19, 2020 06:13
@ijjk
Copy link
Contributor Author

ijjk commented Feb 19, 2020

Thanks for helping get this fix in! We're using watchpack@beta where this fix was specifically needed so if it's available there we should be good without backporting for now 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants