fs: expose BigIntStats, StatFs, FSWatcher, StatWatcher classes
#51681
Conversation
nodejs-github-bot
added
fs
|
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 |
|
@joyeecheung Speaking personally, I like being able to As a bit of extra color, I often use general type-checking helpers that let me say stuff like |
|
I'm -1 |
|
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 |
|
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. |
|
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 |
|
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 |
|
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 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. |
|
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). |
|
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 |
|
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 |
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. |
|
For For 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 If we still want to keep it internal, encourage duck typing checks, and/or implement For the As for potential usecases, I can think of extending/mocking |
I would prefer that we just deprecate direct access to Lines 540 to 553 in 46ce278 If we manage to remove it from the public API we can just do
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.
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 |
PR-URL: #51879 Refs: #51681 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: nodejs#51879 Refs: nodejs#51681 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
PR-URL: #51879 Refs: #51681 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
|
After this #51879 I have warnings (
The problem is only in the latest UPD: |
|
After #51879, this PR is obsolete. |
|
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. |
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.classesobject and define these properties on it (similar tofs.constants) and maybe deprecatefs.Stats,fs.Dir,fs.Dirent,fs.ReadStream,fs.Stats, andfs.WriteStream.Fixes: #51674