Skip to content
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

Allow ArrayBuffer as argument to writeFile and friends #42228

Closed
Jamesernator opened this issue Mar 6, 2022 · 10 comments · May be fixed by #46490
Closed

Allow ArrayBuffer as argument to writeFile and friends #42228

Jamesernator opened this issue Mar 6, 2022 · 10 comments · May be fixed by #46490
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. stale

Comments

@Jamesernator
Copy link

What is the problem this feature will solve?

Currently one can write a large number of types to a file. However one cannot currently write an ArrayBuffer to a file without wrapping it in a typed array.

It would be convenient to be able to write an ArrayBuffer directly to a file if one is received from some library without having to wrap directly. (The fact this isn't possible is something I forget pretty much everytime I go to write an ArrayBuffer).

What is the feature you are proposing to solve the problem?

I propose allowing fs.promises.writeFile("./some-file", someArrayBuffer) to work when someArrayBuffer is an ArrayBuffer. Typed arrays and such are already supported, so this shouldn't be particularly complicated to implement.

What alternatives have you considered?

No response

@Jamesernator Jamesernator added the feature request Issues that request new features to be added to Node.js. label Mar 6, 2022
@VoltrexKeyva VoltrexKeyva added the fs Issues and PRs related to the fs subsystem / file system. label Mar 6, 2022
@JckXia
Copy link
Member

JckXia commented Mar 6, 2022

Looking at the writeFile implementation in (https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L809), it looks like Node enforce the data to be either a TypedArray, iterable, a string (or an object that we can serialize into a string). Not sure if there's a lot of wiggle room to loosen the check against pure ArrayBuffers.

I also wonder if writing arrayBuffers directly to a file is considered to be an undefined behaviour, since they are byte arrays and their representation is dependent on the width of the view we are casting it to. (https://stackoverflow.com/questions/42416783/where-to-use-arraybuffer-vs-typed-array-in-javascript)

@Jamesernator
Copy link
Author

Jamesernator commented Mar 7, 2022

Looking at the writeFile implementation in (https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L809), it looks like Node enforce the data to be either a TypedArray, iterable, a string (or an object that we can serialize into a string). Not sure if there's a lot of wiggle room to loosen the check against pure ArrayBuffers.

The implementation literally just casts any TypedArray into a Uint8Array: (https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L395). There should be no difficulty in just adding an else if (isArrayBuffer(data)) data = new Uint8Array(data).

I also wonder if writing arrayBuffers directly to a file is considered to be an undefined behaviour, since they are byte arrays and their representation is dependent on the width of the view we are casting it to. (https://stackoverflow.com/questions/42416783/where-to-use-arraybuffer-vs-typed-array-in-javascript)

The answer you link would suggest ArrayBuffer is the more correct concept to read/write files with. From the answer:

They are on the other hand used for binary data transfers between server and client, or from the user's file system via Blobs.

Which makes sense, like you actually write a chunk of bytes to a file, not a sequence of Float32 or whatever TypedArray you might happen to have. Blob (and it's subclass File), and Body do in fact expose their data via blobOrBody.arrayBuffer() not via typed arrays.

@JckXia
Copy link
Member

JckXia commented Mar 7, 2022

Okay that makes sense, thanks for the explanation!

@benjamingr
Copy link
Member

@nodejs/fs

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 4, 2022
@gengjiawen gengjiawen removed the stale label Sep 7, 2022
@Jamesernator
Copy link
Author

Jamesernator commented Sep 18, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

keepalive

@SRHerzog
Copy link
Contributor

SRHerzog commented Feb 4, 2023

Here's a draft PR to consider. It required some work in a few different places to apply the change to most of the write functions:

  • fsPromises.writeFile
  • fsPromises.write
  • filehandle.writeFile
  • filehandle.write
  • fs.writeFile
  • fs.writeFileSync
  • fs.write

I didn't touch any writev functions yet, they'll take some more work.

SRHerzog added a commit to SRHerzog/node that referenced this issue Feb 17, 2023
Extended most file writing functions to accept ArrayBuffers rather
than only strings, buffers, or data views. Does not include
functions that accept arrays of dataviews, such as writev.

Fixes: nodejs#42228
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Aug 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 4, 2023
@knowhatamine
Copy link

Given the security risk unchecked readfile with callback is, this seems like a really bad idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. stale
Projects
Status: Pending Triage
Development

Successfully merging a pull request may close this issue.

7 participants