-
-
Notifications
You must be signed in to change notification settings - Fork 51
refactor: allow download as stream #137
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
Conversation
Hi @inian!! Is any change necessary for approval? I think this is a very good feature |
Bump, this would be really useful |
Hi @xxRockOnxx! 👋 Thank you for this PR - adding streaming support is a valuable feature that the community has been requesting for a while (as evidenced by the interest in #72). After discussing this internally, we think the API could be even cleaner with a slightly different approach. Instead of using an options flag, we'd prefer one of these patterns: Option 1: Separate methods// Regular download (returns Blob)
const { data: blob } = await storage.from(bucket).download(path)
// Stream download (returns ReadableStream)
const { data: stream } = await storage.from(bucket).downloadAsStream(path) Option 2: Chain method (preferred)// Regular download (returns Blob)
const { data: blob } = await storage.from(bucket).download(path)
// Stream download (returns ReadableStream)
const { data: stream } = await storage.from(bucket).download(path).asStream() The chain method approach (Option 2) would be our preference as it provides a clean, intuitive API that's discoverable through TypeScript autocompletion. This would involve:
Would you be willing to update your implementation to use one of these patterns? We really appreciate your contribution and look forward to getting this feature merged! Let us know if you have any questions or need any help with the implementation. Thanks again! 🚀 |
1cd6bf4
to
cbc6251
Compare
@mandarini the preferred changes has been implemented. I will edit the OP to reflect this change. |
Hi @xxRockOnxx! Thanks for implementing the fluent API pattern as requested! The API design looks great and aligns perfectly with what we discussed! However, it looks like the new Let me know if you need any help or have questions. |
9f617cc
to
e44d6d1
Compare
@mandarini sorry. i was about to sleep when i pushed that. i now added the missing files. Also I'm not sure if "Builder" is the right word to call these because it seems unnatural to If preferred, I don't mind changing it to Option 1 instead. |
🚀 Preview Release Statusfalse Last updated: 2025-09-12T12:50:01Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
@xxRockOnxx oh no!! |
@mandarini i now included error handling tests for stream download and it works perfectly when running I'm not sure how i can replicate the coverage test with your setup without pinging you for it. |
Thank you very much for your contribution @xxRockOnxx ! |
@mandarini a little off-topic, may I send you an email for it instead? |
@xxRockOnxx you can reach out on our Discord: https://discord.supabase.com/ |
🎉 This PR is included in version 2.12.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
Refactor? Feature?
What is the current behavior?
Related to #72
Right now the library only allows download as
Blob
.What is the new behavior?
This PR allows downloading as stream.
The following code is made possible:
Additional context
This is done by returning a class that either implements
PromiseLike<Blob>
orPromiseLike<ReadableStream>
.Thus 2 new classes were added.
I have considered adding 1 class only such that there will only be
DownloadBuilder<Blob | ReadableStream>
but it may cause confusion because the following will be possible (core code only for brevity):Alternative approaches considered:
asStream()
is called twice → pushes errors to runtime instead of compile-timethis
when already a stream builder → still confusingHaving 2 classes simply makes this impossible to happen.