-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
fs: fix fs.openAsBlob, add fs.openAsBlobSync and fsPromises.openAsBlob
#49759
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
1174c94
a48718b
fe64e58
0b246e4
5e87bbb
319061b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,6 @@ const { | |
| ObjectDefineProperties, | ||
| ObjectDefineProperty, | ||
| Promise, | ||
| PromiseResolve, | ||
| ReflectApply, | ||
| SafeMap, | ||
| SafeSet, | ||
|
|
@@ -572,17 +571,49 @@ function openSync(path, flags, mode) { | |
| * @param {{ | ||
| * type?: string; | ||
| * }} [options] | ||
| * @returns {Promise<Blob>} | ||
| * @param {( | ||
| * err?: Error, | ||
| * blob?: Blob | ||
| * ) => any} callback | ||
| * @returns {void} | ||
| */ | ||
| function openAsBlob(path, options = kEmptyObject) { | ||
| function openAsBlob(path, options, callback) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Help me understand - why do we actually need an async API with a callback for this if it's always called synchronously?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it's highly likely that they will do something with the blob if they have called 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() // worksso i don't think the type, size, filename and lastModified date should be changeable after having called I don't think it should be possible to call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kinda see the point of deferring the reading operations: the methods that allow reading content like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) |
||
| if (callback === undefined) { | ||
| callback = options; | ||
| options = kEmptyObject; | ||
| } else { | ||
| options ??= kEmptyObject; | ||
| } | ||
|
|
||
| validateObject(options, 'options'); | ||
| const type = options.type || ''; | ||
| validateString(type, 'options.type'); | ||
| // 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(pathModule.toNamespacedPath(path), { type })); | ||
anonrig marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| validateFunction(callback, 'cb'); | ||
|
|
||
| let blob; | ||
| let cbError = null; | ||
| try { | ||
| blob = createBlobFromFilePath(pathModule.toNamespacedPath(path), { type }); | ||
| } catch (err) { | ||
| // Permission errors should be thrown instead of being passed to callback | ||
| if (err.code === 'ERR_ACCESS_DENIED') { | ||
| throw err; | ||
| } | ||
| cbError = err; | ||
| } | ||
|
|
||
| process.nextTick(callback, cbError, blob); | ||
| } | ||
|
|
||
| /** | ||
| * @param {string | Buffer | URL } path | ||
| * @param {{ | ||
| * type?: string; | ||
| * }} [options] | ||
| * @returns {Blob} | ||
| */ | ||
| function openAsBlobSync(path, options) { | ||
| return syncFs.openAsBlob(path, options); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -3093,6 +3124,7 @@ module.exports = fs = { | |
| open, | ||
| openSync, | ||
| openAsBlob, | ||
| openAsBlobSync, | ||
| readdir, | ||
| readdirSync, | ||
| read, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.