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.

Sign up for GitHub

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

Jump to bottom

fs: allow setting Stats/BigIntStats Date properties #52708

Merged
merged 5 commits into from Apr 28, 2024
Merged

fs: allow setting Stats/BigIntStats Date properties #52708

merged 5 commits into from Apr 28, 2024

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Apr 26, 2024 •
edited

Fixes #52707, fixes #52705

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 26, 2024
@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft April 26, 2024 13:38
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review April 26, 2024 13:42
@RedYetiDev
Copy link
Member

@nodejs/fs

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Apr 26, 2024
Merged
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58718/

@LiviaMedeiros
Copy link
Contributor

This seems to fix the same bug for BigIntStats, how about a test with { bigint: true }?

@nicolo-ribaudo
Copy link
Contributor Author

@LiviaMedeiros Done!

@nicolo-ribaudo nicolo-ribaudo changed the title fs: allow setting Stat date properties fs: allow setting Stats/BigIntStats Date properties Apr 26, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58720/

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Apr 26, 2024
Merged
@nicolo-ribaudo
Copy link
Contributor Author

Could somebody restart CI? :)

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58727/

@zandercymatics
Copy link

@nicolo-ribaudo Thanks for implementing this fix!

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58730/

mcollina
mcollina approved these changes Apr 27, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@LiviaMedeiros LiviaMedeiros added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2024
@sepehrst
Copy link

In addition to the setter, shouldn't ObjectDefineProperty also be retained in the getter (with writable: true, of course)? Otherwise it would remain lazy on repeated access unless it is written to

@nicolo-ribaudo
Copy link
Contributor Author

The getter calls the setter to define the property (so that the logic doesn't need to be duplicated).

@mcollina mcollina removed the needs-ci PRs that need a full CI run. label Apr 27, 2024
@richardlau richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 27, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 28, 2024
@nodejs-github-bot nodejs-github-bot merged commit 1aab22e into nodejs:main Apr 28, 2024
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1aab22e

@richardlau richardlau added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v21.x PRs that should not land on the v21.x-staging branch and should not be released in v21.x. labels Apr 28, 2024
@nicolo-ribaudo nicolo-ribaudo deleted the fix-22-stat-regression branch April 28, 2024 16:36
@matz3 matz3 mentioned this pull request Apr 29, 2024
Closed
@abroddrick
Copy link

Hey, any idea when this will make it into a "latest" release? My project is having this gulp issue and I'm wondering if this gets put into a latest release soon we don't have to worry about doing a workaround. Thanks for any update you have!

@mcollina
Copy link
Member

this will ship in max 2-3 weeks.

@abroddrick
Copy link

Sounds good! Thanks for the quick response!

aduh95 pushed a commit that referenced this pull request Apr 29, 2024
@nicolo-ribaudo @aduh95
fs: allow setting Stat date properties
c6e0fe2 
PR-URL: #52708
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@zandercymatics zandercymatics mentioned this pull request Apr 29, 2024
Open
2 tasks
@cclauss cclauss mentioned this pull request Apr 30, 2024
Open
@oddhack oddhack mentioned this pull request Apr 30, 2024
Closed
oddhack added a commit to KhronosGroup/Vulkan-Site that referenced this pull request Apr 30, 2024
@oddhack
Use Node.js 20.12.2 instead of 'latest'
9da3131 
Attempt to work around nodejs/node#52708 /
gulpjs/vinyl-fs#350 by reverting to previous
LTS version.
@oddhack oddhack mentioned this pull request Apr 30, 2024
Merged
oddhack added a commit to KhronosGroup/Vulkan-Site that referenced this pull request Apr 30, 2024
@oddhack
Use Node.js 20.12.2 instead of 'latest' (#83)
b690f5a 
Attempt to work around nodejs/node#52708 /
gulpjs/vinyl-fs#350 by reverting to previous
LTS version.
@aduh95 aduh95 mentioned this pull request Apr 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@anonrig anonrig anonrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@rluvaton rluvaton rluvaton approved these changes

@RedYetiDev RedYetiDev RedYetiDev approved these changes

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v21.x PRs that should not land on the v21.x-staging branch and should not be released in v21.x. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone