-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fs/promises API inconsitency (.close not there) #20548
Comments
The patch for adding async function close(handle) {
validateFileHandle(handle);
return handle.close();
} Was it omitted for some actual reason? |
@ChALkeR I believe the design is such that it automatically closes when the handle is gc'd so manually closing it is kinda useless |
@devsnek It's there on the Also, manual closing is not useless, as close-on-gc could be delayed. |
closing manually is still the preferred choice, even when using |
@ChALkeR ... the reason there's no |
@jasnell What's the difference with |
@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.
IMO, we should either export |
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 |
Oooh, this will simplify #20439, so yes please! :-D |
@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 |
(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.) |
After thinking about this, my personal opinion is that, if the API would be kept as it is now (separate from |
This drops exporting duplicate methods that accept FileHandle as the first argument (to mirror callback-based methods accepting 'fd'). Those methods were not adding actual value to the API because all of those are already present as FileHandle methods, and they would probably be confusing to the new users and making docs harder to read. Also, the API was a bit inconsistent and lacked .close(handle). Fixes: nodejs#20548 PR-URL: nodejs#20559 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This drops exporting duplicate methods that accept FileHandle as the first argument (to mirror callback-based methods accepting 'fd'). Those methods were not adding actual value to the API because all of those are already present as FileHandle methods, and they would probably be confusing to the new users and making docs harder to read. Also, the API was a bit inconsistent and lacked .close(handle). Fixes: nodejs#20548 PR-URL: nodejs#20559 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This drops exporting duplicate methods that accept FileHandle as the first argument (to mirror callback-based methods accepting 'fd'). Those methods were not adding actual value to the API because all of those are already present as FileHandle methods, and they would probably be confusing to the new users and making docs harder to read. Also, the API was a bit inconsistent and lacked .close(handle). Fixes: #20548 PR-URL: #20559 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Backport-PR-URL: #21172
This drops exporting duplicate methods that accept FileHandle as the first argument (to mirror callback-based methods accepting 'fd'). Those methods were not adding actual value to the API because all of those are already present as FileHandle methods, and they would probably be confusing to the new users and making docs harder to read. Also, the API was a bit inconsistent and lacked .close(handle). Fixes: #20548 PR-URL: #20559 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Backport-PR-URL: #21172
Currently,
fs/promises
provides a nearly identical API tofs
, minus callbacks → promises change (obvious) and thefd
→filenandle
change (mostly invisible in most usecases).The following parts of
fs
API are not present onfs/promises
:*Sync
methods (obvious)exists
— kinda ok, since it was deprecated and never worked in a promisified variant.access
is 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 onfilehandle
objects and in callback-basedfs
API are also present directly as methods offs/promises
API, accepting afilehandle
. 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 promisifiedfs
versions.I don't see how
close
is different from otherfilehandle
APIs, so in my opinion either of there two should happen:fs/promises
that accept afilehandle
should be dropped in favor of the same methods onfilehandle
objects.This will avoid overcomplicating things, will declutter the docs and will save time future users figuring out the «canonical» way to do something.
fs/promises
should have as much of directfs
API duplicates as reasonable — basically, what we have now +.close
should be added.This will ease migration for people who are already using the current
fs
API with (or without) promisify./cc @jasnell @Trott @BridgeAR
The text was updated successfully, but these errors were encountered: