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

Fix bug where fs.exists is incorrectly promisified #835

Merged
merged 1 commit into from Mar 16, 2021

Conversation

Hexxeh
Copy link
Contributor

@Hexxeh Hexxeh commented Mar 12, 2021

Rollup Plugin Name: node-resolve

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Using the node-resolve plugin in a browser where the fs module is mocked fails to find resolve any paths. Upon debugging this issue, it appears to be because fs.exists is passed to util.promisify. The Node docs explicitly call out this function as being non-standard and not following the pattern for util.promisify to work correctly.

As far as I can tell, this works on Node because fs.exists actually internally already provides a promisified version via: https://nodejs.org/api/util.html#util_custom_promisified_functions

Using stat, which doesn't have this non-standard-ness, works around this issue.

I haven't included tests because this fixes a bug when running in an admittedly quite strange setup that's currently not replicated in tests at all as far as I know.

export const readFile = promisify(fs.readFile);
export const realpath = promisify(fs.realpath);
export { realpathSync } from 'fs';
export const stat = promisify(fs.stat);
export async function exists(filePath) {
Copy link

@merceyz merceyz Mar 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since fs.exists is just a wrapper around fs.access you should use that to make sure the behaviour is the same and to not unnecessarily create the stat object just to throw it away

https://github.com/nodejs/node/blob/5ff2e582cd867feba71bb13d9d8bed9953721145/lib/fs.js#L240-L252

export const readFile = promisify(fs.readFile);
export const realpath = promisify(fs.realpath);
export { realpathSync } from 'fs';
export const stat = promisify(fs.stat);
export async function exists(filePath) {
try {
await stat(filePath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is "access" function to check file existence.

@TrySound
Copy link
Member

Also looks like already promisified version are available in node 10
https://nodejs.org/api/fs.html#fs_fspromises_access_path_mode

Copy link
Contributor

@danielgindi danielgindi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using stat (or even better, access) makes more sense anyway, as exists is kind of deprecated in node. It's recommended to use a more informative method that tells what the thing is (file/folder/other), if it's accessible to you, etc.

@Hexxeh Hexxeh force-pushed the lmcloughlin/exists-promisify branch from 0b912c3 to 4b58114 Compare March 15, 2021 15:44
@Hexxeh
Copy link
Contributor Author

Hexxeh commented Mar 15, 2021

Swapped stat for access per comments. Thanks for the reviews!

@shellscape shellscape merged commit 312fbc2 into rollup:master Mar 16, 2021
@shellscape
Copy link
Collaborator

thanks!

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

Successfully merging this pull request may close these issues.

None yet

6 participants