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

Support splitting of files and directories in getTimeInfoEntries #205

Merged
merged 3 commits into from Nov 24, 2021

Conversation

markjm
Copy link
Contributor

@markjm markjm commented Nov 17, 2021

Supports the splitting of getTimeInfoEntries between files and directories.
This supports both old and new functionality:

// a complete map of all timestamps, exactly the same as before these changes
const times = w.getTimeInfoEntries()

// filling in of two seperate maps for files and directories
const files = new Map();
const directories = new Map();
w.getTimeInfoEntries(files, directories)

As a small note, the caching present in old mechanism is removed. From my investigation, it doesnt actually block out much computation (since we still need to do the map merging one recursion-level up). The caching also would not provide any benefit in the new mechanism, as we need to do the same number of map additions regardless.

Given these changes, we can incorporate into webpack by changing using the new map-passing pattern here. I have those changes all ready to go, and all webpack-side watching tests are passing with my linked version of these changes.
https://github.com/webpack/webpack/blob/c181294865dca01b28e6e316636fef5f2aad4eb6/lib/node/NodeWatchFileSystem.js#L81

@markjm
Copy link
Contributor Author

markjm commented Nov 17, 2021

Azure Pipelines tests all pass, but need permissions for Travis-CI

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #205 (4c9c76f) into main (e71b62b) will increase coverage by 1.41%.
The diff coverage is 94.73%.

❗ Current head 4c9c76f differs from pull request most recent head 8d14e94. Consider uploading reports for the commit 8d14e94 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
+ Coverage   91.38%   92.80%   +1.41%     
==========================================
  Files           6        6              
  Lines        1068     1014      -54     
  Branches      257      239      -18     
==========================================
- Hits          976      941      -35     
+ Misses         92       73      -19     
Flag Coverage Δ
integration 92.80% <94.73%> (+1.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/DirectoryWatcher.js 92.74% <90.00%> (+1.08%) ⬆️
lib/watchpack.js 95.87% <100.00%> (+4.36%) ⬆️

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 e71b62b...8d14e94. Read the comment docs.

@markjm
Copy link
Contributor Author

markjm commented Nov 17, 2021

Thanks @vankop for queuing - I foolishly left an it.only call in there - hence the huge loss of coverage. Fixed in latest push (force push to clean up commit history)

avoid deeply collecting watchers since they are always collected during time collection

add existence file time info for directories

fix safeTime collection
@sokra sokra merged commit 89d5f48 into webpack:main Nov 24, 2021
@sokra
Copy link
Member

sokra commented Nov 24, 2021

Thanks

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