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

Supabase - Storage driver #19135

Merged
merged 36 commits into from
Jul 25, 2023

Conversation

matt-rolley
Copy link
Contributor

Hey,

I've attempted to add a Supabase storage driver based on the S3 storage driver (where possible). It should help close #16903.

Please let me know if anything needs changing or if I've missed a step (it's my first proper pull request :)) - happy to make any changes that are needed.

Thanks,
Matt

@changeset-bot
Copy link

changeset-bot bot commented Jul 11, 2023

🦋 Changeset detected

Latest commit: 7c3314c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@directus/storage-driver-supabase Major
@directus/api Minor
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matt-rolley matt-rolley changed the title Supabase - Storage driver #16903 Supabase - Storage driver Jul 12, 2023
@paescuj paescuj requested review from a team, paescuj, br41nslug and azrikahar and removed request for a team July 12, 2023 10:11
@pixelkoduje
Copy link

@matt-rolley Hello, how to run your version to test? Because it is not known if and when she will enter the main directus quest. And I really need it.

@rijkvanzanten rijkvanzanten added this to the Next Release milestone Jul 23, 2023
@matt-rolley
Copy link
Contributor Author

Sorry, I've been away for a week so just catching up properly - appreciate the updates and apologies if I've stepped on any toes, I just thought it'd be an interesting feature to add.

Hoping to contribute more in the future so please let me know if there is a better way for me to help out :)

@rijkvanzanten
Copy link
Member

Sorry, I've been away for a week so just catching up properly - appreciate the updates and apologies if I've stepped on any toes, I just thought it'd be an interesting feature to add.

Oh no not at all! Really appreciate this PR and the functionalities it unlocks 😄 My comment earlier was just to explain to @pixelkoduje why running an open source project can't just "just merge it already" yolo mode all the time 😬 I'm aiming to have it reviewed by this weeks release though!

Hoping to contribute more in the future so please let me know if there is a better way for me to help out :)

Thanks! PRs are welcome for any Issue (which includes Supabase storage support!) 🙂 Happy to chat on Discord if you want to talk through specifics.

@rijkvanzanten
Copy link
Member

@matt-rolley I'm curious to hear your thoughts on resumable uploads in this context. Where does Upload from tus keep track of the previous parts? Seeing that the Directus API itself doesn't have resumable uploads, does it make sense for this abstraction to have it? Should we instead remove the resumable uploads from the Supabase driver specifically in favor of making it natively supported across the driver abstraction layer

@matt-rolley
Copy link
Contributor Author

@matt-rolley I'm curious to hear your thoughts on resumable uploads in this context. Where does Upload from tus keep track of the previous parts? Seeing that the Directus API itself doesn't have resumable uploads, does it make sense for this abstraction to have it? Should we instead remove the resumable uploads from the Supabase driver specifically in favor of making it natively supported across the driver abstraction layer

To be honest I'm not 100% sure how it works, I just saw that Supabase supported resumable uploads so I added it into the storage layer.

I think resumable uploads would be a great feature, especially for the larger files, but if it'd only be supported on the Supabase storage layer then I agree that it makes more sense to add it to the driver abstraction layer. Does it make sense to remove it from this version until it's better tested/added to the driver abstraction layer?

@rijkvanzanten
Copy link
Member

Does it make sense to remove it from this version until it's better tested/added to the driver abstraction layer?

Yeah lets do that 👍🏻 Resumable uploads is really something that should be implemented in the Directus API first, and then we can pass that information on to the Supabase (or any other!) driver. I don't think it'll really do anything in it's current state, as Directus will cancel the upload on unexpected errors (rather than retry)

@matt-rolley
Copy link
Contributor Author

Yeah, agreed - sounds like a plan. I've removed it from the codebase now :)

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

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

This is looking and feeling great @matt-rolley! 🚀

Really appreciate how closely you've matched the existing code styles and paradigms used. If I didn't know any better, I could've thought this was written by myself 😄

@rijkvanzanten rijkvanzanten merged commit f9635ac into directus:main Jul 25, 2023
8 checks passed
@rijkvanzanten rijkvanzanten self-assigned this Jul 25, 2023
@dosandco
Copy link

This is super exciting - thanks so much guys!

@matt-rolley matt-rolley deleted the storage-driver-supabase branch July 25, 2023 15:37
@matt-rolley
Copy link
Contributor Author

You're welcome :) That's great, really glad it's working nicely!

Looking forward to the release, if any issues pop up, feel free to tag me in them :) happy to help where I can.

Thanks again for the help in getting this live.

br-rafaelbarros pushed a commit to personal-forks/directus-source that referenced this pull request Nov 7, 2023
* Initial commit to add supabase as a storage driver

* Working supabase implementation

* Slight cleanup

* Started working on tests for storage-driver-supabase

* Updates

* Initial commit to add supabase as a storage driver

* Working supabase implementation

* Slight cleanup

* Started working on tests for storage-driver-supabase

* Updates

* Nicer working tests

* More working tests

* Readable stream

* Working version with all passing tests and types

* Update contributors.yml

* added documentation

* Ran prettier to fix formatting issues

* Ran linter and added Supabase to dictionary

* Fix tsconfig reference

* Organize imports

* Start at v0

* Add changeset

* Lock versions

* storage-driver-supabse: Added fullpath for move and copy arguments

* storage-driver-supabase: Removed resumable uploads

* Update lockfile

---------

Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add native Supabase Storage abstraction
6 participants