fs: expose BigIntStats, StatFs, FSWatcher, StatWatcher classes

[LiviaMedeiros]

These classes are documented in https://nodejs.org/api/fs.html#common-objects as a part of public API.

Alternatively, we can add common fs.classes object and define these properties on it (similar to fs.constants) and maybe deprecate fs.Stats, fs.Dir, fs.Dirent, fs.ReadStream, fs.Stats, and fs.WriteStream.

Fixes: #51674

[joyeecheung]

I don't think it's a good idea to export them unless there is a use case that we cannot fix without exporting them directly (for instance checks I think adding something like fs.isStats() would already be enough), otherwise we can increase the breakage surface and it makes it difficult to e.g. change the constructor of the stats objects.

[danfuzz]

@joyeecheung Speaking personally, I like being able to instanceof things and not having to remember when that is and isn't possible, and then remember what the oddball alternative is in the latter case. I know I have to use Array.isArray() because of very old historical reasons, but it seems unfortunate to me to increase the number of special cases that one has to remember.

As a bit of extra color, I often use general type-checking helpers that let me say stuff like MustBe.instanceOf(obj, classObj), which produces reasaonbly nice error messages. This can't work unless the class objects are exposed. I know I can work around this by manually exposing the class myself (e.g. exportingfs.statSync('/', {bigint: true}).constructor from my type-checker module), but I'd really prefer not to.

[marco-ippolito]

I'm -1
I think it's a mistake to expose internals unless stricly needed, it makes it very hard to evolve. If we exposed those, we would have to go through the whole cycle of deprecation like is happening in many objects of crypto, when we decide to refactor.

[danfuzz]

I 100% agree about not wanting to expose internals. In this case, however, the class in question doesn't feel like an internal per se, at least to me. What I mean is that actual instances of this class are directly exposed, and the class of those instances is bona fide useful information as the instances pass further away from the call site that generated them.

I also understand the concern about potential refactoring, in general. In the case that I filed a bug for, though, I am surprised that exporting BigIntStats — just like Stats is already exported — would make the difference between an easy and onerous refactoring, should that ever come to pass.

[joyeecheung]

I think the main concern is more about exporting the constructor and unfortunately in JavaScript the class is the constructor. For example the BigInt stats objects are currently constructed in JS land with all the fields computed from a BigInt64Array first (to avoid cross-layer costs) passed as arguments. If we should find passing BigInts as arguments to be a lot slower than passing the BigInt64Array as argument directly/doing it in any other way differently, if the constructor is exported we'd have to do a semver-major to change what the constructor accepts, even though it's a mere optimization. This doesn't seem to be worth it when what's really needed is an instance check, not the constructor itself.

[danfuzz]

Would you all ever consider an arrangement where the exposed constructor creates an empty instance, and the system uses an effectively private method (e.g. named with a private/uninterned symbol) to do the actual initialization work? This would let you expose the class without also exposing the sorts of implementation details you're concerned about.

I know this isn't quite how it'd look in Node, but I mean something like this:

const privateNew = Symbol('privateNew');

class BigIntStats extends StatsBase {
  static [privateNew](dev, mode, nlink, uid, gid, rdev, blksize,
      ino, size, blocks, atimeNs, mtimeNs, ctimeNs, birthtimeNs) {
    const obj = new this();
    obj.dev = dev;
    ...
    obj.atimeMs = atimeNs / kNsPerMsBigInt;
    ...

    return obj;
  }
  ...
}

then instead of new BigIntStats(...) the use site in getStatsFromBinding() would be BigIntStats[privateNew](...).

[joyeecheung]

If we try that hard to prevent a constructor from being used as a constructor, it seems strange to me that we expose the constructor in the first place. If the only use case is just type checking, I'd still prefer that we expose a method like fs.isStats(), which could also work cross-realm (the same reason why Array.isArray() is preferred over instanceof Array).

[danfuzz]

The reason to expose the constructor is exactly for all the reasons a constructor is useful in JavaScript other than actually constructing.

I will admit I find it baffling that the consensus would be to prefer ad-hoc methods over good-ole instanceof. My take is that Array.isArray() is necessary but unfortunate, and isn't something I'd want to see more of. It's not just that folks will have to remember isStats() per se, but also that it messes with the larger ecosystem too; just as one example, with Jest I won't be able to say expect(x).isInstanceOf(BigIntStats) which would give a helpful error message if violated. Not saying I can't deal, of course — I have. I will. — just that this strikes me as a slippery slope that doesn't end up with Node in a better place than other alternatives.

I actually thought my suggestion had a chance of being acceptable, because it solves exactly the problem as stated, is easy to implement, and doesn't particularly bloat the system.

[joyeecheung]

On the usage of jest assertion, it seems to me what you actually need is an extension in the jest API to customize your type checker so that you can do something like expect(x).isInstanceOf('Stats object', fs.isStats) and when it fails it tells you that the object you are passing isn't a "Stats object" (or any description you have) with your type checker. That would be way more flexible than requiring the instanceof check to work (and, in addition, requiring that the constructor has a readable name that the API can grok somehow - it's also common practices to construct classes in factories, even with anonymous class expressions extending a parent class, etc. and I think in that case Jest would tell you the target type is Function or "constructor name is not a target string"). On the other hand relying on instanceof for type check in the API is bound to have many quirks whenever the user uses vm contexts and ShadowRealms (we also have a follow-up NodeRealm API in progress, so the pitfall of instanceof is only going to be more and more obvious).

[danfuzz]

Not trying to argue the point anymore, but hoping to understand: I don't see why ShadowRealms affect this at all. I didn't think behavior-bearing object references were allowed to cross realm boundaries, so how would you end up with something coming from another realm for which an isStats() call could even make a determination about?

[joyeecheung]

By the way, I wonder why users would have to explicitly check the instance type of BigIntStats. This is technically in the duck typing territory - there is essentially no guarantee that the object returned must be constructed by a constructor. Node.js can also simply return an object with the necessary fields & methods and that's still part of the API contract (not that we need to do it now but this gives us more freedom in the optimization). Even our own tests don't check the constructor at all and just check the fields and see if "it squeaks like a duck", e.g.

node/test/parallel/test-fs-promises.js

Lines 86 to 97 in c41cf6f

	function verifyStatFsObject(stat, isBigint = false) {
	const valueType = isBigint ? 'bigint' : 'number';
	assert.strictEqual(typeof stat, 'object');
	assert.strictEqual(typeof stat.type, valueType);
	assert.strictEqual(typeof stat.bsize, valueType);
	assert.strictEqual(typeof stat.blocks, valueType);
	assert.strictEqual(typeof stat.bfree, valueType);
	assert.strictEqual(typeof stat.bavail, valueType);
	assert.strictEqual(typeof stat.files, valueType);
	assert.strictEqual(typeof stat.ffree, valueType);
	}

[joyeecheung]

I didn't think behavior-bearing object references were allowed to cross realm boundaries, so how would you end up with something coming from another realm for which an isStats() call could even make a determination about?

That restriction is just for shadow realms, there is ongoing work to provide that through a new NodeRealm API too. The integration of shadow realms already makes it possible to bootstrap a different fs implementation in a new vm context now, we are just not exposing them yet, but that's expected to happen when shadow realm goes stable.

[LiviaMedeiros]

For StatFs, FSWatcher, StatWatcher, I agree that we shouldn't expose class if it's not needed in userspace; and the fact there's no issue asking for them (AFAIK) implies they are not needed. Maybe fs documentation can be a bit more explicit about that, e.g. by not referring to them as fs exports/properties.

For BigIntStats, so far I don't see rationale behind exposing Stats but not BigIntStats.

The semverity, imho, is not a big problem here: if/when refactor happens, it might be better to be explicit about it, rather than breaking de-facto accessible-from-userspace constructor. The documentation also never defines constructor signature, users must somehow (either guess and test, or look at source code) know the exact order of fields to do new Stats(...) in userspace, and it's a questionable idea whether constructor is exposed or not. And if they do need new Stats() or extends Stats, they most likely will need new BigIntStats() and extends BigIntStats as well.

If we still want to keep it internal, encourage duck typing checks, and/or implement isStats(), should we consider deprecating fs.Stats as well for the very same reasons?

For the fs.isStats(), I wouldn't block it but feel -0.5. It would be a bit confusing (it should provide instanceof Stats and instanceof BigIntStats independently, at least), it would be a weird function to expose on fs.promises (or inconvenient to use for folks who import node:fs/promises if we don't), and it would solve only the instanceof usecase.

As for potential usecases, I can think of extending/mocking fs. E.g. making fs.stat('myremotefs://192.168.12.34/path/to/file', { bigint: true }) that returns MyRemoteFsStats which extends fs.Stats; or mocked fs.stat('/path/to/remote/file/as/if/local') that fetches file from predefined https server and tries its best to build Stats instance from corresponding HTTP headers. I'm not aware of such projects though, it's just something we can have if constructor gets documented.

[joyeecheung]

If we still want to keep it internal, encourage duck typing checks, and/or implement isStats(), should we consider deprecating fs.Stats as well for the very same reasons?

I would prefer that we just deprecate direct access to fs.Stats. There is a trend deprecating similar direct access to constructors that are not supposed to be invoked directly. e.g. #51077 to enable internal refactoring without having to go semver-major for small changes in the constructor. Being exposed to fs directly is why the Stats implementation still looks like this in 2024 (this constructor predates ES6):

node/lib/internal/fs/utils.js

Lines 540 to 553 in 46ce278

	function Stats(dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks,
	atimeMs, mtimeMs, ctimeMs, birthtimeMs) {
	FunctionPrototypeCall(StatsBase, this, dev, mode, nlink, uid, gid, rdev,
	blksize, ino, size, blocks);
	this.atimeMs = atimeMs;
	this.mtimeMs = mtimeMs;
	this.ctimeMs = ctimeMs;
	this.birthtimeMs = birthtimeMs;
	}
	ObjectSetPrototypeOf(Stats.prototype, StatsBase.prototype);
	ObjectSetPrototypeOf(Stats, StatsBase);
	ObjectDefineProperties(Stats.prototype, lazyDateFields);

If we manage to remove it from the public API we can just do Stats extends StatsBase here (which is a breaking change by disallowing construct calls without new/Function.prototype.call - and if we want to start breaking it, we might as well just remove it from fs and stop worrying about semverity when we refactor it once and for all).

The semverity, imho, is not a big problem here: if/when refactor happens, it might be better to be explicit about it, rather than breaking de-facto accessible-from-userspace constructor.

I don't think we've ever considered constructors that can only be accessed indirectly to be part of the semver contract. Exposing the constructors directly from built-ins is a lot more encouraging for user-land dependency than only allowing users to access them indirectly, IMO. And breaking changes to the former deserves a semver-major a lot more than the latter.

As for potential usecases, I can think of extending/mocking fs

I think this is where duck typing would be preferred - and as far as I know it's a common practice to just mock objects with duck typing anyway, since the JS language is practically built for this. Users almost certainly would've preferred overriding the methods instead of inheriting the original ones when they mock the Stats object anyway - otherwise they need careful calculations of the internal fields for the inherited methods to work, and there is no compatibility guarantee about how the built-in calculation is done so it can easily break whenever Node.js is updated. If we encourage users to extend fs.Stats we are also practically encouraging them to rely on e.g. the existing implementation of Stats.prototype._checkModeProperty remaining constant forever.

[AlttiRi]

After this #51879 I have warnings (TS6385: Stats is deprecated.) in IDE now.

I totally dislike them.

---

The problem is only in the latest "@types/node": "20.12.13" (20.12.12 have no that deprecation).

---

UPD:
I reported this bug here DefinitelyTyped/DefinitelyTyped#69713

[LiviaMedeiros]

After #51879, this PR is obsolete. fs.Stats should not be accessed from userspace anymore.

[joyeecheung]

That looks like an issue of the types/node package. They should be defined as separate interfaces like other objects from Node.js without an actual class, instead of being actual builtin classes now. The types package is not maintained by Node.js, you can file an issue to their issue tracker.