Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add fs.exists[util.promisify.custom] #8936

Merged
merged 3 commits into from
Feb 17, 2024
Merged

Add fs.exists[util.promisify.custom] #8936

merged 3 commits into from
Feb 17, 2024

Conversation

dmitri-gb
Copy link
Contributor

@dmitri-gb dmitri-gb commented Feb 16, 2024

What does this PR do?

This adds the missing implementation for fs.exists[util.promisify.custom].

fs.exists doesn't follow the error-first-callback convention, so it needs a custom implementation for util.promisify. Without it, a promisifed exists will always reject (with true or false as the reason).

Prisma CLI relies on this.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote automated tests.

  • I included a test for the new code, or existing tests cover it
  • I ran my tests locally and they pass (bun-debug test test-file-name.test)

fs.exists doesn't follow the error-first-callback convention, so it
needs a custom implementation for util.promisify.
return false;
}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Relavant Node.js code:
https://github.com/nodejs/node/blob/53aed8838cb71142ca1591b655000aac90d91205/lib/fs.js#L276-L281

ObjectDefineProperty(exists, kCustomPromisifiedSymbol, {
  __proto__: null,
  value: function exists(path) { // eslint-disable-line func-name-matching
    return new Promise((resolve) => fs.exists(path, resolve));
  },
});

💡 Might be implemented this way in Node because of its older perf history with await.
Of interest is @Jarred-Sumner's twitter thread on JSC await perf improvements.

Node's doesn't suppress the in the promise form because it's already suppressing errors in fs.exists (as do we):
https://github.com/nodejs/node/blob/53aed8838cb71142ca1591b655000aac90d91205/lib/fs.js#L265-L273

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdalton are you saying I should remove the try-catch? I only added it because I'm not really sure about the semantics of Bun.fs().exists (whether it ever throws/rejects etc) and also because the callback variant defintion adds a rejection handler:

fs.exists.$apply(fs, [path]).then(
  existed => callback(existed),
  _ => callback(false),
);

Copy link
Contributor

Choose a reason for hiding this comment

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

That bun is handling the rejection and returning false aligns with expectations. The try-catch it's wrapped in helps for any synchronous issues so both(sync/async) are covered. No need to wrap it in additional try-catches. I'd just tweak to do as node does. It's a single line and keeps things simple:

new Promise((resolve) => fs.exists(path, resolve))

in a non async function.

@jdalton jdalton merged commit fe8ec29 into oven-sh:main Feb 17, 2024
26 of 31 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants