fs: fix fs.openAsBlob, add fs.openAsBlobSync and fsPromises.openAsBlob

[LiviaMedeiros]

fs.openAsBlob() was added in #45258 as part of Callback API despite returning Promise instead of calling callback.

I guess it should look more like this?

semver-minor PRs that contain new features and should be released in the next minor version. unless there are objections (fs.openAsBlob was marked as experimental).

cc @jasnell

[LiviaMedeiros]

cc @nodejs/fs

[benjamingr]

Help me understand - why do we actually need an async API with a callback for this if it's always called synchronously?

[LiviaMedeiros]

Just to have it aligned on API level. Having a function that pretends (at least, for now) to be async is less weird that not having it in other APIs at all.

I would like to see this PR superseded or followed up with proper implementation and maybe switch from Blob to File, but judging from #45258 (comment) the former seems to be problematic.

[benjamingr]

I miss @addaleax 😍 , I think she was spot on and the impl shouldn't use blocking I/O here and return a promise/take a callback that's quite confusing IMO.

Additionally I think in the case of Blob we can probably return immediately and defer access to the file to the point someone actually tries to do something with the blob. That can be synchronous and perform no I/O and would arguably be a better API (since it makes no assumptions about the file in-between creating the Blob and accessing it)

[jimmywarting]

defer access to the file to the point someone actually tries to do something with the blob.

I think it's highly likely that they will do something with the blob if they have called openAsBlob
i think it should check if the file exist and throw a DOMexception NotFoundError otherwise.
it's not a case about if but when they will do any I/O operation after having called openAsBlob. Developers are most likely going to use it at some point. So why not just do it upfront right away rather then later? it's going to do all this work anyway

It should also guard against modifications afterwards.

const blob = openAsBlob(path)
fs.appendFileSync(path, data)
blob.text() // ups, should throw DOMException, detected mtime or size changes. 

I believe browser do also support renaming the file and moving it to another folder and still being readable afterwards. think i remember testing this a long time ago but can't remember if that was the case.

const blob = openAsBlob(path)
fs.moveSync(path, dest)
blob.text() // works

so i don't think the type, size, filename and lastModified date should be changeable after having called openAsBlob so that would involve having to stat the file and look up some properties for it.

I don't think it should be possible to call openAsBlob(path) on a path that dose not exist, create the file and then do a blob.text() afterwards. that would just be weird and unexpected.

[LiviaMedeiros]

I kinda see the point of deferring the reading operations: the methods that allow reading content like Blob.prototype.arrayBuffer, Blob.prototype.text are async, so we only need stats to be determined immediately.
In case if I plan to read it via Blob.prototype.stream, I would be surprised to see that Node.js swallows whole file and keeps it in memory before even constructing the stream. After all, it's called openAsBlob and not readAsBlob.

[jimmywarting]

After all, it's called openAsBlob and not readAsBlob.

Yup! Blob/File are more like FileSystemFileHandle or fs.FileHandle that just points to somewhere where it can locate and read the data from. (and what start/end offset it has)

a blob don't read anything to memory (or at least it should not - unless it's constructed from memory)

[benjamingr]

If this is to mimmic the promise API and pretend to be async this needs a process.nextTick or queueMicrotask

[jasnell]

I'm fine with exposing the method on fs.promises but I don't think the existing method on fs should be changed and definitely don't think there needs to be a openAsBlobSync.

[jimmywarting]

don't think there needs to be a openAsBlobSync

Care to elaborate why?

[LiviaMedeiros]

I'd like to see elaboration on why we should have promise-based method in Callback API as well.

[jasnell]

There's really not a lot to ellaborate on. I don't think there's value in changing the existing API and there's definitely no value in having an openAsBlobSync. I simply view this as an unnecessary change.