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

WIP: Add conditional support for gfs.promises. #166

Closed
wants to merge 1 commit into from

Conversation

coreyfarrell
Copy link
Collaborator

This support is dependent on util.promisify existing, so node.js 8+.

Fixes #160


@isaacs I think the code itself is complete, I want to seek feedback before I spend the time on more test coverage. Of note unlike native fs.promises, reading the gfs.promises property will never produce an 'experimental' warning. Additionally native fs.promises is not available in node.js 8 or 9, gfs.promises is available in those versions. The list of functions that are promisified is from Object.keys(require('fs').promises).sort() in node.js 12.7.0.

One difference, require('fs').promises appears to be separate wrappers around internal functions. So fs.promises.stat is not the result of util.promisify(fs.stat). Technically speaking the correct way to deal with this would be to reimplement all patched functions based directly on fs.promises but that seems like it would be a maintenance burden and would limit use of gfs.promises to the versions of node.js which provide fs.promises. I'm not sure if there is someone from node.js we should ask if this makes an actual difference in any cases or if the node.js implementation does the same thing as util.promisify but with better performance.

This support is dependent on `util.promisify` existing, so node.js 8+.

Fixes isaacs#160
@coreyfarrell
Copy link
Collaborator Author

I should have spent more time in the documentation, it does appear that fs.promises has some differences that are not insignificant. I'm having second thoughts about the wisdom of implementing gfs.promises in this way. Might be better to instead provide this as gfs.promisified or just not do this at all. I'm thinking if gfs.promises is provided it should operate on FileHandle objects which would limit the feature to node.js 10+ and would not prevent the experimental warning from versions of node.js which provide it.

@coreyfarrell
Copy link
Collaborator Author

Abandoning for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs Promises API
1 participant