-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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(ext/node): Add fs.lutimes
/ fs.lutimesSync
#23172
Conversation
The node_compat test is failing on the windows runner due to a permission denied error when making (or maybe removing?) a temp file. I don't have a windows machine to debug this on, but it's odd that it only fails on the |
7fc13bc
to
82bae74
Compare
f9a79f3
to
c64ac28
Compare
CI issue has been fixed, so this is now ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Part of denoland#18218 - Adds `fs.lutimes` and `fs.lutimesSync` to our node polyfills. To do this I added methods to the `FileSystem` trait + ops to expose the functionality to JS. - Exports `fs._toUnixTimestamp`. Node exposes an internal util `toUnixTimestamp` from the fs module to be used by unit tests (so we need it for the unit test to pass unmodified). It's weird because it's only supposed to be used internally but it's still publicly accessible - Matches up error handling and timestamp handling for fs.futimes and fs.utimes with node - Enables the node_compat utimes test - this exercises futimes, lutimes, and utimes.
Part of #18218
fs.lutimes
andfs.lutimesSync
to our node polyfills. To do this I added methods to theFileSystem
trait + ops to expose the functionality to JS.fs._toUnixTimestamp
. Node exposes an internal utiltoUnixTimestamp
from the fs module to be used by unit tests (so we need it for the unit test to pass unmodified). It's weird because it's only supposed to be used internally but it's still publicly accessible