Skip to content

fs: allow setting Stats/BigIntStats Date properties - #52708

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
nicolo-ribaudo:fix-22-stat-regression
Apr 28, 2024
Merged

fs: allow setting Stats/BigIntStats Date properties#52708
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
nicolo-ribaudo:fix-22-stat-regression

Conversation

@nicolo-ribaudo

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

Copy link
Copy Markdown
Contributor

Fixes#52707, fixes#52705

@nodejs-github-botnodejs-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
@avivkeller

avivkeller commented Apr 26, 2024

Copy link
Copy Markdown
Member

@nodejs/fs

@nicolo-ribaudonicolo-ribaudo mentioned this pull request Apr 26, 2024
Merged
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

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

@LiviaMedeiros

LiviaMedeiros commented Apr 26, 2024

Copy link
Copy Markdown
Member

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

@nicolo-ribaudo

nicolo-ribaudo commented Apr 26, 2024

Copy link
Copy Markdown
ContributorAuthor

@LiviaMedeiros Done!

@nicolo-ribaudonicolo-ribaudo changed the title fs: allow setting Stat date propertiesfs: allow setting Stats/BigIntStats Date propertiesApr 26, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

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

@nicolo-ribaudonicolo-ribaudo mentioned this pull request Apr 26, 2024
Merged
@nicolo-ribaudo

nicolo-ribaudo commented Apr 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Could somebody restart CI? :)

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

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

@zandercymatics

zandercymatics commented Apr 26, 2024

Copy link
Copy Markdown

@nicolo-ribaudo Thanks for implementing this fix!

@nodejs-github-bot

nodejs-github-bot commented Apr 26, 2024

Copy link
Copy Markdown
Collaborator

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

@LiviaMedeirosLiviaMedeiros 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

sepehrst commented Apr 27, 2024

Copy link
Copy Markdown

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

nicolo-ribaudo commented Apr 27, 2024

Copy link
Copy Markdown
ContributorAuthor

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

@mcollinamcollina removed the needs-ci PRs that need a full CI run. label Apr 27, 2024
@richardlaurichardlau 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-botnodejs-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:mainApr 28, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 28, 2024

Copy link
Copy Markdown
Collaborator

Landed in 1aab22e

@nicolo-ribaudo
nicolo-ribaudo deleted the fix-22-stat-regression branch April 28, 2024 16:36
@matz3matz3 mentioned this pull request Apr 29, 2024
Closed
@allly-b

allly-b commented Apr 29, 2024

Copy link
Copy Markdown

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

mcollina commented Apr 29, 2024

Copy link
Copy Markdown
Member

this will ship in max 2-3 weeks.

@allly-b

allly-b commented Apr 29, 2024

Copy link
Copy Markdown

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>
@zandercymaticszandercymatics mentioned this pull request Apr 29, 2024
Closed
2 tasks
@cclausscclauss mentioned this pull request Apr 30, 2024
Closed
@oddhackoddhack 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.
@oddhackoddhack 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.
@aduh95aduh95 mentioned this pull request Apr 30, 2024
Merged
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@nicolo-ribaudo@soophoo
fs: allow setting Stat date properties
ec97813
PR-URL: nodejs#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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@anonriganonriganonrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@rluvatonrluvatonrluvaton approved these changes
@avivkelleravivkelleravivkeller approved these changes
@LiviaMedeirosLiviaMedeirosLiviaMedeiros approved these changes
+1 more reviewer
@VoltrexKeyvaVoltrexKeyvaVoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

15 participants

@nicolo-ribaudo@avivkeller@nodejs-github-bot@LiviaMedeiros@zandercymatics@sepehrst@allly-b@mcollina@jasnell@lpinca@anonrig@joyeecheung@rluvaton@VoltrexKeyva@richardlau