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

Support file uploads using a stream #252

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

Conversation

mike-marcacci
Copy link
Contributor

@mike-marcacci mike-marcacci commented Sep 10, 2024

The current implementation requires a file to exist in the local filesystem, which is an uncommon scenario for production use cases. Instead, files are typically either stored in an appropriate external storage system like GCS or are supplied directly by the user over the network.

This change adds the ability to supply a file upload as a stream, and improves the existing filesystem flow by streaming the file's contents rather than attempting to load it into memory first (it was previously using the blocking call, which is only really appropriate for init stage file loading, quick demos, or systems that don't need to support concurrent requests).

The current implementation requires a file to exist in the local
filesystem, which is an uncommon scenario for production use cases.

This adds the ability to supply the upload contents as a node stream,
and improves the filesystem case by streaming instead of loading into
memory.
@mike-marcacci
Copy link
Contributor Author

Heyo - I just want to check whether a streaming approach is something that will be considered in this project. We've been patching this locally during development, but if this is unlikely to be merged or made possible through an alternate implementation, we'll just go ahead and publish a fork of this package.

@mike-marcacci
Copy link
Contributor Author

In case it's helpful to anyone, here's a pretty straightforward workaround, largely just copying the existing implementation.

@CamdenClark
Copy link

Really appreciate you trying to PR this, and thank you for the workaround.

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