Skip to content
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

Fully decorated fs.promises for complete fs replacement? #28545

Closed
rvagg opened this issue Jul 5, 2019 · 8 comments
Closed

Fully decorated fs.promises for complete fs replacement? #28545

rvagg opened this issue Jul 5, 2019 · 8 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises.

Comments

@rvagg
Copy link
Member

rvagg commented Jul 5, 2019

I've got into the habit of const fs = require('fs').promises, which is fine for the most part for nice async / await code. But since it's not decorated with the fs properties other than promisified versions of the core functions that usefulness breaks down.

The main one is fs.constants, the other ones don't tend to be used directly but there is a case to be made for exposing them too.

With the current interface I can't do this kind of thing, which would implement an await touch(file):

const fs = require('fs').promises

module.exports = function touch (f) {
  return fs.access(f, fs.constants.F_OK).catch(() => fs.writeFile(f, Buffer.alloc(0)))
}

I can achieve this if I manually decorate it with fs.constants = require('fs').constants. So it's not super terrible, just inconvenient and means that require('fs').promises isn't a fully viable require('fs') replacement for async functions.

Has this come up? Have we come up with a consistent philosophy for fs.promises that would preclude this? Or should I (or someone else) cook up a PR to make this work with the more fs decorations? constants at least.

Current decorations excluding the standard functions and promises are:

  • fs.Dirent
  • fs.Stats
  • fs.ReadStream
  • fs.WriteStream
  • fs.FileReadStream
  • fs.FileWriteStream
  • fs._toUnixTimestamp (could be excluded)
  • fs.F_OK (could be excluded, I guess these 4 are here for historical reasons)
  • fs.R_OK
  • fs.W_OK
  • fs.X_OK
  • fs.constants
@rvagg rvagg added fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises. labels Jul 5, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Jul 5, 2019

I ran into the same problem. I personally think it would be best to just add another set of properties to the main part with Promise or something like that in their name.
The current implementation also causes problems with esm and the names are not distinguishable from their callback counterparts. This is not intuitive for me and having unique names fixes all of these issues.

@devsnek
Copy link
Member

devsnek commented Jul 5, 2019

adding promise to the name would be rather annoying for continued usage though. +1 to adding the constants and such to the promises object.

@rvagg
Copy link
Member Author

rvagg commented Jul 5, 2019

To take this a bit further, I forgot to include createReadStream and createWriteStream because I have precisely the same problem with those (and just ran into it, which reminded me!).

Here's the full list of what's in fs that's not in fs.promises

  • fs.close
  • fs.createReadStream
  • fs.createWriteStream
  • fs.exists
  • fs.fchown
  • fs.fchmod
  • fs.fdatasync
  • fs.fstat
  • fs.fsync
  • fs.ftruncate
  • fs.futimes
  • fs.read
  • fs.unwatchFile
  • fs.watch
  • fs.watchFile
  • fs.write
  • fs.Dirent
  • fs.Stats
  • fs.ReadStream
  • fs.WriteStream
  • fs.FileReadStream
  • fs.FileWriteStream
  • fs._toUnixTimestamp
  • fs.F_OK
  • fs.R_OK
  • fs.W_OK
  • fs.X_OK
  • fs.constants

What I want to be able to do is just const fs = require('fs').promises and be done with it. @BridgeAR I think your suggestion means just const fs = require('fs') and then fs.writeFilePromise() etc.? I guess that'd be OK, but we have fs.promises right now being widely used so we're sort of stuck with it no?

Regarding problems with ESM, can you elaborate? Does having a .promises cause problems and are we having regrets? Does this create blockers for expanding on fs.promises as it exists now?

@DRoet
Copy link

DRoet commented Jul 5, 2019

@rvagg Currently in ESM you have to do something like this:

import { promises as fsPromises } from 'fs';
const { readFile } = fsPromises;

due to the way it is exposed, see #28466

@silverwind
Copy link
Contributor

I probably sound insane, but I'd also like to see sync methods exported on fs.promises because I often have to mix sync/async fs to workaround the non-existant top-level await in Node.js and doing two imports just sucks.

@silverwind
Copy link
Contributor

silverwind commented Jul 5, 2019

In hindsight, I'm very unhappy with fs.promises in general. Maybe it's time to do a proper migration (e.g. make both callback and promise work at the same time for a limited time). A good example how such a migration could be done is electron.

@silverwind silverwind added the discuss Issues opened for discussions and feedbacks. label Jul 5, 2019
@rvagg
Copy link
Member Author

rvagg commented Jul 5, 2019

That was discussed a lot at the time and decided pretty overwhelmingly that the breakage just wasn't worth it. Not all these functions are easily switchable on last argument presence (exists being the best example). But regardless, this issue isn't a question about large scale redesigns, that's not going to get anywhere in any meaningful amount of time. I just want a well decorated fs.promises for now. Perhaps something more radical can be exported for ESM or under a namespace. That's not this issue though.

@silverwind silverwind removed the discuss Issues opened for discussions and feedbacks. label Jul 6, 2019
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

There's been no activity on this for nearly a year. Many of the functions on fs just don't make sense in the promise version because they've been moved into the FileHandle object (e.g. fileHandle.read() for instance). If someone wishes to move anything forward on this, a PR would be the best approach. Leaving the issue open here doesn't seem to be doing any good. Closing. Can reopen if anyone wants to revisit the discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

No branches or pull requests

6 participants