Skip to content

fs: fix always return promise from fs.openAsBlob #62655

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.

Sign up for GitHub

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

Open
mattskel wants to merge 3 commits into
base: main
Choose a base branch
from
+28 −2
Open

fs: fix always return promise from fs.openAsBlob #62655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a4c236
fs: always return promise from openAsBlob()
mattskel Apr 9, 2026
d42280d
fs: always return promise form openAsBlob()
mattskel Apr 20, 2026
08c326a
fs: return promise when error throws
mattskel Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const {
ObjectDefineProperty,
Promise,
PromisePrototypeThen,
PromiseReject,
PromiseResolve,
ReflectApply,
SafeMap,
Expand Down Expand Up @@ -574,8 +575,12 @@ function openAsBlob(path, options = kEmptyObject) {
// The underlying implementation here returns the Blob synchronously for now.
// To give ourselves flexibility to maybe return the Blob asynchronously,
// this API returns a Promise.
path = getValidatedPath(path);
return PromiseResolve(createBlobFromFilePath(path, { type }));
Copy link
Copy Markdown
Contributor

@Slayer95 Slayer95 Apr 17, 2026

Choose a reason for hiding this comment

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

This can be done in a one-liner by substituting PromiseResolve for PromiseTry nowadays. (Though idk whether the primordial for Promise.try is already defined.)

Copy link
Copy Markdown
Author

@mattskel mattskel Apr 20, 2026

Choose a reason for hiding this comment

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

@Slayer95 I tried implementing Promise.try. I tried adding the primordial PromiseTry but from what I could tell Promise.try was not available when primordials were built. I assume this is due to it being a relatively new feature. I also tried using the Promise primordial directly and this did work, but from what I understand the Promise primordial is not safe from monkey-patching so I abandoned this approach.

I have updated the approach to wrap the entire functional block in try/catch. I think this is more inline with the original issue than the previous implementation and ensures the function always returns a promise. Plus I think to achieve that with the PromiseTry approach could be a bit hard to read.

I also replaced Promise.reject with the primordial for consistency.

Let me know what you think of this updated approach.

Copy link
Copy Markdown
Contributor

@Slayer95 Slayer95 Apr 20, 2026

Choose a reason for hiding this comment

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

Your new approach would be equivalent to just making openAsBlob async, wouldn't it?

Now, however, there are two error-handling camps:

  1. Programmer errors due to bad inputs should always fail as early as possible. Even better: crash and exit the program.
  2. APIs should error in a consistent way. Only error codes / only throws / only rejections / only error events / etc.

While I sympathize with camp 1, I think mixing throws and rejections is a huge no-no, since it's effectively mixing sync with async, violating function coloring rules.

Copy link
Copy Markdown
Author

@mattskel mattskel Apr 21, 2026

Choose a reason for hiding this comment

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

I have reverted the try/catch changes as well as added tests.

try {
path = getValidatedPath(path);
return PromiseResolve(createBlobFromFilePath(path, { type }));
} catch (error) {
return PromiseReject(error);
}
}

/**
Expand Down
21 changes: 21 additions & 0 deletions test/parallel/test-fs-openasblob-promise-contract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

const assert = require('assert');
const fs = require('fs');

// Verify invalid paths throw synchronously and return a Promise
(async () => {
let promise;
assert.doesNotThrow(() => { promise = fs.openAsBlob('does-not-exist'); });
assert.strictEqual(typeof promise?.then, 'function');
await assert.rejects(promise, { code: 'ERR_INVALID_ARG_VALUE' });
})();


// Verify that invalid arguments throw synchronously and return a Promise
(async () => {
let promise;
assert.doesNotThrow(() => { promise = fs.openAsBlob(123); });
assert.strictEqual(typeof promise?.then, 'function');
await assert.rejects(promise, { code: 'ERR_INVALID_ARG_TYPE' });
})();