Skip to content

Support splitting of files and directories in getTimeInfoEntries#205

Merged
sokra merged 3 commits intowebpack:mainfrom
markjm:markjm/split
Nov 24, 2021
Merged

Support splitting of files and directories in getTimeInfoEntries#205
sokra merged 3 commits intowebpack:mainfrom
markjm:markjm/split

Conversation

@markjm
Copy link
Copy Markdown

@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
Copy Markdown
Author

markjm commented Nov 17, 2021

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

@codecov
Copy link
Copy Markdown

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

2 participants