fs: fix always return promise from fs.openAsBlob#62655
fs: fix always return promise from fs.openAsBlob#62655mattskel wants to merge 2 commits intonodejs:mainfrom
fs.openAsBlob#62655Conversation
Wrap path validation and blob creation in try/catch. Any synchronous errors become Promise rejections. Ensures the function always returns a Promise. Fixes: nodejs#62418
nodejs-github-bot
added
fs
| // To give ourselves flexibility to maybe return the Blob asynchronously, | ||
| // this API returns a Promise. | ||
| path = getValidatedPath(path); | ||
| return PromiseResolve(createBlobFromFilePath(path, { type })); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Your new approach would be equivalent to just making openAsBlob async, wouldn't it?
Now, however, there are two error-handling camps:
- Programmer errors due to bad inputs should always fail as early as possible. Even better: crash and exit the program.
- 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.
Wrap entire function in try/catch. Ensure that the function always returns promise. Replace Promise.reject with primordial.
3 participants
Summary: Ensure
fs.openAsBlobalways returns a Promise by converting synchronous validation/creation errors into Promise rejections so error handling is consistent.Motivation: The issue reported that openAsBlob can throw synchronously (for invalid paths), which bypasses
await/.catch. This aligns behaviour with the Promise-based API contract.Changes: Wrap getValidatedPath and createBlobFromFilePath in
try/catchand return Promise.reject on failure.Fixes: #62418