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

Respect FS_ACCURACY when determining if file is untouched #215

Merged
merged 1 commit into from
May 17, 2022

Conversation

markjm
Copy link
Contributor

@markjm markjm commented Apr 14, 2022

Should resolve webpack/webpack#15431

After noodling on the discussions there and looking at past similar work done (namely 164007c), I think this is the best way to resolve that issue while still doing best-effort to respect file systems with lower mtime accuracy.

Right now, the if check I am changing has an arbitrary 1s, while everywhere else is properly using the FS_ACCURACY, which will get updated to higher resolution if possible.

I think this is the best we can do without adding an optional dep of fsevents (fwiw, for OSX, chokidar will either use fsevents, or fall back to polling)

mtime + FS_ACCURACY < now - 1000
=
mtime + FS_ACCURACY + 1000 < now
> replace 1000 with FS_ACCURACY
mtime + FS_ACCURACY + FS_ACCURACY < now
=
mtime + 2* FS_ACCURACY < now

@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #215 (3e59650) into main (15ac38f) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
- Coverage   92.21%   92.12%   -0.09%     
==========================================
  Files           6        6              
  Lines        1040     1041       +1     
  Branches      248      249       +1     
==========================================
  Hits          959      959              
- Misses         81       82       +1     
Flag Coverage Δ
integration 92.12% <100.00%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
lib/DirectoryWatcher.js 92.57% <100.00%> (-0.21%) ⬇️

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 15ac38f...3e59650. Read the comment docs.

@vankop
Copy link
Member

vankop commented Apr 14, 2022

not sure that issue is related to this... It appears for me in some strange circumstances..

@markjm
Copy link
Contributor Author

markjm commented Apr 14, 2022

I am able to repro 100% on M1 mac. The issue seems to be due to fs.watch triggering a change event twice on every save - one as a "typical NOTE_WRITE event, and one due to a NOTE_EXTEND (not a bug as per nodejs/node-v0.x-archive#2054).

This is reproducible with just raw node script

const fs = require("fs");
let i = 0;
fs.writeFileSync("./a.txt", " ")
const watcher = fs.watch("./a.txt", { });
watcher.on("change", (type, filename) => {
    console.log(++i, type, filename, Date.now(), fs.statSync("./a.txt"))
});

setInterval(() => fs.appendFileSync("./a.txt", "x"), 5000)

Based on the commit I linked above, I am following that as precedent for filtering out these double events. This fixes my repro 100% of the time

@markjm
Copy link
Contributor Author

markjm commented Apr 18, 2022

/cc @sokra

@sokra
Copy link
Member

sokra commented Apr 19, 2022

Makes sense to me.

I wonder if the 2 * is even needed. The logic doesn't seem to need that.

Could you try if the problem is also fixed without the 2 *?

@markjm
Copy link
Contributor Author

markjm commented Apr 19, 2022

yep works fine for this issue. Kept the 2* because i wasnt sure if there was some nuance with "windowing" of events. Let me remove it now though.

@markjm markjm force-pushed the markjm/repsect-fs-accuracy branch from 43fb9a9 to 3e59650 Compare April 19, 2022 23:07
@markjm
Copy link
Contributor Author

markjm commented May 2, 2022

@sokra this good to go? some checks seem stuck, but idk if i am able to requeue them

@vankop vankop closed this May 2, 2022
@vankop vankop reopened this May 2, 2022
@buondevid
Copy link

We were experiencing double compiling on single file changes during watch mode. We worked around it with aggregateTimeout to 100 but I can confirm that this patch, which I tested locally, fixes the issue. Thanks 😉.

@markjm
Copy link
Contributor Author

markjm commented May 12, 2022

Glad the patch is working for you. I'm ready to merge this and update deps in webpack whenever!

@sokra sokra merged commit a54bcdb into webpack:main May 17, 2022
@sokra
Copy link
Member

sokra commented May 17, 2022

Thanks

@fireairforce
Copy link

fireairforce commented May 19, 2022

Could u provide a patch version for this change? @sokra thank u~
I use this patch at my local environment, and it works fine for me

@tondi
Copy link

tondi commented May 26, 2022

Can we have this published? 🙏🏻

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.

webpack watch mode rebuild twice on file change [MacOS]
6 participants