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

Bugfix in calculation of modified time of files in watch mode #3198

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

ryanwilsonperkin
Copy link
Contributor

@ryanwilsonperkin ryanwilsonperkin commented Apr 8, 2022

What kind of change does this PR introduce?

Bugfix

Did you add tests for your changes?

No, seemed too niche for a test case but happy to add one with a bit of instruction (namely whether test/plugin/plugin.test.js is the correct spot)

If relevant, did you update the documentation?

Not relevant, updates a verbose log

Summary

Fixes #3197

We need to pass the timestamp in milliseconds to the date constructor, this code was assuming that changeTime was in seconds and needed to be multiplied by 1000 to get milliseconds but it is already in milliseconds.

This led to the constructed date being incorrect.

Before After
image

Does this PR introduce a breaking change?

No

Other information

It appears that the changeTime value comes through the WatchMethod value of WatchFileSystem type, ultimately tracing back to the watchpack package. Watchpack uses fs.lstat to retrieve a fs.Stat object of the file and retuns the mtime by casting it to a number using the + operand. This might cause some platform-dependant behaviour, such as if +new Date() returns different levels of precision between Mac/Windows/Linux/etc. There's a mtimeMs on the Stat object that might be more appropriate in this case to ensure a certain level of precision is returned.

Ref: https://nodejs.org/api/fs.html#stat-time-values

We need to pass the timestamp in milliseconds to the date constructor,
this code was assuming that changeTime was in seconds and needed to be
multiplied by 1000 to get milliseconds but it is already in
milliseconds.

This led to the constructed date being incorrect.
@ryanwilsonperkin ryanwilsonperkin requested a review from a team as a code owner April 8, 2022 14:12
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ryanwilsonperkin / name: Ryan Wilson-Perkin (4751a2d)

@ryanwilsonperkin ryanwilsonperkin changed the title fix: changeTime is already in milliseconds Bugfix in calculation of modified time of files in watch mode Apr 8, 2022
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #3198 (4751a2d) into master (6d778a5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3198   +/-   ##
=======================================
  Coverage   90.98%   90.98%           
=======================================
  Files          23       23           
  Lines        1731     1731           
  Branches      519      519           
=======================================
  Hits         1575     1575           
  Misses        156      156           
Impacted Files Coverage Δ
packages/webpack-cli/src/plugins/CLIPlugin.ts 100.00% <100.00%> (ø)

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 6d778a5...4751a2d. Read the comment docs.

Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, lets wait for CI to be green.

@rishabh3112
Copy link
Member

/cc @webpack/cli-team need one more review

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@rishabh3112 rishabh3112 merged commit d390d32 into webpack:master Apr 11, 2022
@rishabh3112
Copy link
Member

Thanks

@ryanwilsonperkin ryanwilsonperkin deleted the fix-changed-time-bug branch April 11, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timestamp in verbose logs is incorrect by a factor of 1000x
4 participants