Use async/await for internal readAsync function. NFC #23120
Conversation
| #if ASSERTIONS | ||
| assert(!binary || Buffer.isBuffer(ret)); | ||
| #endif | ||
| return ret; |
There was a problem hiding this comment.
The async keyword automatically makes the return value into a promise here.
| setTimeout(() => resolve(readBinary(f))); | ||
| }); | ||
| }; | ||
| readAsync = async (f) => readBinary(f); |
There was a problem hiding this comment.
The async keyword automatically makes the return value into a promise here.
| @@ -9,18 +9,17 @@ readBinary = (filename) => { | |||
| filename = isFileURI(filename) ? new URL(filename) : filename; | |||
| var ret = fs.readFileSync(filename); | |||
| #if ASSERTIONS | |||
| assert(ret.buffer); | |||
| assert(Buffer.isBuffer(ret)); | |||
There was a problem hiding this comment.
I think this is more accurately reflects what this assertion was trying to prove. i.e. that the returned value is a node Buffer object.
There was a problem hiding this comment.
That sounds right, but I recall some differences between node versions... we do have coverage of multiple ones on CI though, so if it passes we should be ok I guess.
There was a problem hiding this comment.
I was trying to figure out what this assert could possibly be for... is it to detect older node versions that don't return a buffer? If so, shouldn't we just do that with the version check? Buffer.isBuffer has been around forever so I certainly think it should be fine to use: https://nodejs.org/api/buffer.html#static-method-bufferisbufferobj
There was a problem hiding this comment.
Oh right, yeah, it was whether a Buffer was returned or not. Yeah, this lgtm.
sbc100
force-pushed
the
readAsync
branch
3 times, most recently
from
90b6983 to
8bf08af
Compare
| @@ -9,18 +9,17 @@ readBinary = (filename) => { | |||
| filename = isFileURI(filename) ? new URL(filename) : filename; | |||
| var ret = fs.readFileSync(filename); | |||
| #if ASSERTIONS | |||
| assert(ret.buffer); | |||
| assert(Buffer.isBuffer(ret)); | |||
There was a problem hiding this comment.
That sounds right, but I recall some differences between node versions... we do have coverage of multiple ones on CI though, so if it passes we should be ok I guess.
Followup to #23104.