Skip to content

fs: add Temporal.Instant support to Stats and BigIntStats#60789

Open
LiviaMedeiros wants to merge 6 commits into
nodejs:mainfrom
LiviaMedeiros:fs-stats-temporal
Open

fs: add Temporal.Instant support to Stats and BigIntStats#60789
LiviaMedeiros wants to merge 6 commits into
nodejs:mainfrom
LiviaMedeiros:fs-stats-temporal

Conversation

@LiviaMedeiros
Copy link
Copy Markdown
Member

Refs: #57891

This is a very rough draft, and probably shouldn't land until Temporal implementation is finalized and/or enabled by default.
Opening early to see if there are any objections, suggestions to naming (the *Instant properties are facing userspace and having consistent convention would be nice), or to implementation.

@LiviaMedeiros LiviaMedeiros added fs Issues and PRs related to the fs subsystem / file system. experimental Issues and PRs related to experimental features. labels Nov 20, 2025
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 20, 2025
@Renegade334
Copy link
Copy Markdown
Member

Renegade334 commented Nov 20, 2025

Since Date.prototype.toInstant() comes along with Temporal, these are fairly trivial to obtain anyway? (Although come to think of it, this would lose nanosecond precision.)

It might be better to have this as a switchable option similar to bigint, rather than adding the overhead of building a load of Temporal objects for every stats call?

@LiviaMedeiros
Copy link
Copy Markdown
Member Author

Since Date.prototype.toInstant() comes along with Temporal, these are fairly trivial to obtain anyway?

Converting from Date would have performance overhead on top of precision loss.

It might be better to have this as a switchable option similar to bigint, rather than adding the overhead of building a load of Temporal objects for every stats call?

These properties are lazy-loaded, so the overhead when not used should be minimal.

Comment thread lib/internal/fs/utils.js Outdated
Comment on lines +440 to +441
// TODO(LiviaMedeiros): TemporalInstant primordial
return new Temporal.Instant(BigInt(MathFloor(msec / kMsPerSec)) * kNsPerSecBigInt + BigInt(nsec));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're probably going to want to support building node without Temporal support for a while

Suggested change
// TODO(LiviaMedeiros): TemporalInstant primordial
return new Temporal.Instant(BigInt(MathFloor(msec / kMsPerSec)) * kNsPerSecBigInt + BigInt(nsec));
if (Temporal == null) throw 'informative error';
// TODO(LiviaMedeiros): TemporalInstant primordial
return new Temporal.Instant(BigInt(MathFloor(msec / kMsPerSec)) * kNsPerSecBigInt + BigInt(nsec));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed, especially with mandatory Rust requirements.
Added the error, the message text is subject to changes (if this may land before enabling Temporal by default, it should also suggest --harmony-temporal flag)

Comment thread lib/internal/fs/utils.js Outdated
return this.atimeInstant = instantFromTimeSpecMs(this.atimeMs, this[kPartialAtimeNs]);
},
set(value) {
ObjectDefineProperty(this, 'atimeInstant', { __proto__: null, value, writable: true });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
ObjectDefineProperty(this, 'atimeInstant', { __proto__: null, value, writable: true });
setOwnProperty(this, 'atimeInstant', value);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AFAICT this will make the property enumerable, even though the initial getter is not? I don't mind it, but this better be aligned with how Date setters behave.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Nov 21, 2025

Choose a reason for hiding this comment

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

We should probably add an additional optional enumerable argument to setOwnProperty

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The original getter is enumerable though

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, yes. The difference is that original getter is defined on prototype, so Object.keys() won't list them... I'll open a PR changing enumerability for Dates; if for whatever reason the current behaviour would be preserved, i'd still prefer to align Temporal properties with Date ones.

enumerable argument might be justified if there's enough uses for it, however it's probably out of scope for this PR.

@LiviaMedeiros LiviaMedeiros marked this pull request as ready for review May 14, 2026 20:07
Comment thread doc/api/fs.md Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 87.34177% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.05%. Comparing base (c24e552) to head (315e806).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/utils.js 87.17% 20 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #60789    +/-   ##
========================================
  Coverage   90.05%   90.05%            
========================================
  Files         714      714            
  Lines      225247   225514   +267     
  Branches    42578    42612    +34     
========================================
+ Hits       202842   203092   +250     
- Misses      14181    14211    +30     
+ Partials     8224     8211    -13     
Files with missing lines Coverage Δ
lib/internal/errors.js 97.65% <100.00%> (+<0.01%) ⬆️
lib/internal/fs/utils.js 97.91% <87.17%> (-1.78%) ⬇️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread doc/api/fs.md Outdated
Comment thread lib/internal/fs/utils.js Outdated
enumerable: true,
configurable: true,
get() {
return this.atimeInstant = instantFromTimeSpecMs(this.atimeMs, this[kPartialAtimeNs]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's avoid the additional setter call

Suggested change
return this.atimeInstant = instantFromTimeSpecMs(this.atimeMs, this[kPartialAtimeNs]);
const value = instantFromTimeSpecMs(this.atimeMs, this[kPartialAtimeNs]);
setOwnProperty(this, 'atimeInstant', value);
return value;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure. I guess we can do the same in lazyDateFields getters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With https://github.com/nodejs/node/pull/63328/changes#diff-f70ef1e11e07dbfe54a7470ee790031f214ed08577129d0a9d3891d7310c0a35 (since setOwnProperty() was designed to replace assignment rather than defineOwnProperty(), i believe it should return value itself), these also can be changed back to

-      const value = instantFromTimeSpecMs(this.atimeMs, this[kPartialAtimeNs]);
-      setOwnProperty(this, 'atimeInstant', value);
-      return value;
+     return setOwnProperty(this, 'atimeInstant', instantFromTimeSpecMs(this.atimeMs, this[kPartialAtimeNs]));

Signed-off-by: LiviaMedeiros <livia@cirno.name>
LiviaMedeiros and others added 3 commits May 15, 2026 16:12
Comment thread doc/api/errors.md
@aduh95 aduh95 added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v25.x PRs that should not land on the v25.x-staging branch and should not be released in v25.x. labels May 15, 2026
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v25.x PRs that should not land on the v25.x-staging branch and should not be released in v25.x. experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants