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

Feat/add buffer support #298

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

abdelkd
Copy link

@abdelkd abdelkd commented Nov 16, 2024

This Pull-Request adds support for using Buffer inside of uploadFile function instead of having file to be on local disk. It can be used for example to process a form request and upload immediately instead of saving it on local and then uploading it.

@jimmywarting
Copy link

jimmywarting commented Dec 1, 2024

Thank you for this, this is the kind of smart thinking i like the most!
A package should assume as little about the system it runs on as much as possible.
I sometimes think it's bad that some application assumes it have access to some basic I/O operations when it could in fact run on multiple possible environment, like NodeJS, Bun, Deno, Browsers, Web Workers etc, it can be so sandboxed as much as possible. I might want to operate on the sandboxed file system in the browsers own storage.

a package should basically work as a stdin and stdout only and learn from the onion architecture

One small flaw in this PR is that you used node:Buffer instead of plain Uint8Array or ArrayBuffer instead. I'm not a huge fan of NodeJS specific things. it pollutes the env in other runtimes.

Another suggestion is to instead use Blob instead of buffer/uint8array/arraybuffer. a blob dose not necessary have to hold all data in memory and can be streamed instead. NodeJS, Bun and browser can back up the content of the file from a location on the disc. see eg: https://nodejs.org/api/fs.html#fsopenasblobpath-options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants