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

Straight-forward way to reimplement fs.promises.open #29109

Closed
coreyfarrell opened this issue Aug 13, 2019 · 2 comments
Closed

Straight-forward way to reimplement fs.promises.open #29109

coreyfarrell opened this issue Aug 13, 2019 · 2 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@coreyfarrell
Copy link
Member

Is your feature request related to a problem? Please describe.
I have a PR open against graceful-fs which (among other updates) adds support for fs.promises. A sticky point is the implementation of gracefulFS.promises.open. We need this to create an object that extends FileHandle from lib/internal/fs/promises.js.

Describe the solution you'd like
Directly expose class FileHandle from internal/fs/promises.js as fs.promises.FileHandle to allow extending. Alter the constructor to detect a numeric filehandle:

class FileHandle {
  constructor(filehandle) {
    if (typeof filehandle === 'number') {
      filehandle = new binding.FileHandle(filehandle);
    }
    // validate (filehandle instanceof binding.FileHandle)?
    this[kHandle] = filehandle;
  }

  // ... clipped ...
}

This would allow implementing a user-space fs.promises.open with a function based on a promisified fs.open function. In this scheme setting fs.promises.FileHandle would not alter the functionality of fs.promises.open.

const promiseOpen = promisify(gracefulFS.open);

class GracefulFileHandle extends fs.promises.FileHandle {
  // override methods as needed
}

const myPromises  = {
  ...fs.promises, // simplified copy of default functions for demo
  FileHandle: GracefulFileHandle,
  async open(...args) {
    return new GracefulFileHandle(await promiseOpen(...args));
  }
};

Describe alternatives you've considered
Current PR at isaacs/node-graceful-fs#173 does some very ugly stuff in promises.js to implement fs.promises.open which resolves to an extended class based on fs.promises.FileHandle. First it retrieves the C++ file handle class from process.binding('fs').FileHandle even though process.binding is heading for deprecation. Worse it uses fs.promises.open(__filename, 'r') to create a filehandle which is used to retrieve class FileHandle from lib/internal/fs/promises.js, then extends that class. A temporary promises.open method is created which awaits creation of the real method. This hacky method would be required to support require('graceful-fs').promises for current versions of node.js, but it would be nice if fs.promises.FileHandle could be detected to avoid all the hacks/deprecated API's.

I had considered a completely user-space implementation of gracefulFS.promises based on util.promisify but this would get really messy to reimplement fs.promises functions which accept filehandle, would potentially cause file handles returned by gracefulFS.promises.open to be incompatible with native fs.promises methods. fs.promises.readFile for example accepts a filehandle as the first argument, I'm unsure how it would react to a userspace implementation of the file handle. The user-space implementation would also fail to close FD's upon garbage collection (I don't agree with this auto-close feature but it exists).

@bnoordhuis bnoordhuis added feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. labels Aug 14, 2019
@bnoordhuis
Copy link
Member

bnoordhuis commented Aug 14, 2019

After all the trouble graceful-fs has caused us over the years, I personally am rather disinclined to add hacks for its benefit. I won't pretend to speak for all maintainers but I'm sure I'm not the only one who feels that way.

Even if it wasn't graceful-fs, adding a hack for a single third-party module is unlikely to happen.

@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Closing given the lack of any further activity here.

@jasnell jasnell closed this as completed Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

3 participants