Repository navigation
fs/promises API inconsitency (.close not there) #20548
Description
Currently, fs/promises provides a nearly identical API to fs, minus callbacks → promises change (obvious) and the fd → filenandle change (mostly invisible in most usecases).
The following parts of fs API are not present on fs/promises:
*Syncmethods (obvious)- non-functions (ok)
exists— kinda ok, since it was deprecated and never worked in a promisified variant.accessis there.*Stream— well, kinda ok, not a promise.*watch*— also ok, it doesn't behave like promises.close— why?
All the other methods, except close, that are present on filehandle objects and in callback-based fs API are also present directly as methods of fs/promises API, accepting a filehandle. I assume that was done to ease the migration and provide 1:1 mapping between those where sensible, e.g. for people who were previously using promisified fs versions.
I don't see how close is different from other filehandle APIs, so in my opinion either of there two should happen:
- All the methods of
fs/promisesthat accept afilehandleshould be dropped in favor of the same methods onfilehandleobjects.
This will avoid overcomplicating things, will declutter the docs and will save time future users figuring out the «canonical» way to do something. fs/promisesshould have as much of directfsAPI duplicates as reasonable — basically, what we have now +.closeshould be added.
This will ease migration for people who are already using the currentfsAPI with (or without) promisify.
Activity
[-]fs/promises API inconsitency[/-][+]fs/promises API inconsitency (.close not there)[/+]ChALkeR commentedon May 5, 2018
The patch for adding
closeis trivial, I suppose:Was it omitted for some actual reason?
devsnek commentedon May 5, 2018
@ChALkeR I believe the design is such that it automatically closes when the handle is gc'd so manually closing it is kinda useless
ChALkeR commentedon May 5, 2018
@devsnek It's there on the
filehandlethough: https://nodejs.org/api/fs.html#fs_filehandle_closeAlso, manual closing is not useless, as close-on-gc could be delayed.
jasnell commentedon May 5, 2018
closing manually is still the preferred choice, even when using
FileHandle.prototype.close()... in fact, a warning will be emitted if the handle is closed on gc.jasnell commentedon May 5, 2018
@ChALkeR ... the reason there's no
require('fs/promises').closeis because it's already onFileHandle... it would be redundant and largely unnecessary.ChALkeR commentedon May 5, 2018
@jasnell What's the difference with
write, for example? That reasoning for not includingcloseis equally applicable for not includingwriteandsync(and others).ChALkeR commentedon May 5, 2018
@jasnell Ah, I suppose that I see what you are talking about — but those are mere implementation details mostly hidden from the users, It shouldn't infuence the API needlessly.
fs/promisesexportssyncin two variants:fsync(filehandle)FileHandle#sync(),writein two variants:write(filehandle, ...andFileHandle#write(..., the same for pretty much every method dealing with filehandles. But notclose. What makesclosespecial from the API PoV?IMO, we should either export
close(handle)method or not exportfsync(handle)/write(handle,…/etc.jasnell commentedon May 5, 2018
Ah, right, yeah the variants on FileHandle were added after. I forgot about those. Ugh, yeah, if we're going to have those we might as well have close also
Trott commentedon May 5, 2018
Oooh, this will simplify #20439, so yes please! :-D
ChALkeR commentedon May 5, 2018
@jasnell @Trott As
fs/promisesis experimental, we could also remove methods that accept handles in favor ofFileHande#variants. Both approaches (keeping 2 and switching to a single variant) have upsides and downsides. What do you think?Trott commentedon May 5, 2018
@ChALkeR No opinion from me other than whichever way we go, maximizing consistency in the API would be helpful (to me, at least).
@nodejs/fs
Trott commentedon May 5, 2018
(OK, I guess I do slightly prefer having only one correct way to do something. But that would kind of argue against introducing fs.promises in core in the first place. In fact, it would probably argue against using JavaScript at all. So, again, I'll defer to others.)
10 remaining items
fs: drop duplicate API in promises mode