Skip to content
This repository was archived by the owner on Dec 16, 2025. It is now read-only.

Conversation

@asyncmacro
Copy link
Contributor

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

Copy link
Collaborator

@hkt74 hkt74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the contribution, could you regenerate the api docs to resolve the conflicts?

@hkt74
Copy link
Collaborator

hkt74 commented Mar 3, 2025

I cloned this PR to #365 which resolved the merge conflict

@asyncmacro asyncmacro closed this by deleting the head repository Mar 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants