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: Resumable uploads #294

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

Conversation

juancarlospaco
Copy link
Contributor

@juancarlospaco juancarlospaco commented Aug 28, 2024

What kind of change does this PR introduce?

  • Resumable uploads feature.

@juancarlospaco juancarlospaco marked this pull request as ready for review October 23, 2024 02:28
@juancarlospaco juancarlospaco marked this pull request as draft October 25, 2024 14:22
@juancarlospaco juancarlospaco marked this pull request as ready for review October 29, 2024 00:32
@silentworks silentworks requested a review from fenos October 29, 2024 12:09
@bogdanteleaga
Copy link
Contributor

Will this be merged anytime soon? Experiencing some problems with regular uploads from some instances and this has been suggested as a possible fix in a ticket.

Copy link
Collaborator

@silentworks silentworks left a comment

Choose a reason for hiding this comment

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

I've finally gotten around to reviewing this and there are a few things I think we should change here.

  • The API seems too verbose and requires too many steps to start uploading when compared to https://github.com/tus/tus-py-client or even https://github.com/tus/tus-js-client.
  • I have tested this with local Supabase (and this will probably be the same with self-hosted) and the TUS implementation in these environments don't return an upload-expires header and this is not implemented in any of the other TUS clients I've looked at so we should probably not have this either
  • In the tests you are checking if a url is https, not sure if this is a requirement for uploads as local Supabase doesn't have TLS enabled by default

@silentworks
Copy link
Collaborator

After having a chat with @grdsdev, I think this PR should be put on hold for now. It diverges from the other Supabase client libraries and we should probably wait for an API design to be agreed upon before including resumable uploads into the Python library itself. In the time being we can add example code to the docs https://supabase.com/docs/guides/storage/uploads/resumable-uploads?queryGroups=language&language=js where Resumable uploads is mentioned using one of the third party libraries available for TUS with Python.

@silentworks
Copy link
Collaborator

I've opened a PR with example code for tus-py-client in the docs supabase/supabase#32517 and I've also created a repo with a working example of this https://github.com/silentworks/supabase-tus-upload-examples

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.

3 participants