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

Add retries to Supabase client #382

Merged
merged 15 commits into from
Aug 30, 2024
Merged

Conversation

felixgabler
Copy link
Contributor

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

I wanted something similar to ofetch retries for Supabase client too. This is because we are getting a lot of errors because of flaky requests. I could not figure out how to use ofetch directly for this so I built it manually. Do you have a better idea? Also, how should we make it configurable?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

Copy link

netlify bot commented Jul 6, 2024

Deploy Preview for n3-supabase canceled.

Name Link
🔨 Latest commit 7f9626e
🔍 Latest deploy log https://app.netlify.com/sites/n3-supabase/deploys/66d17d8daf3c75000893f4d5

@larbish
Copy link
Collaborator

larbish commented Jul 9, 2024

@atinux Don't you think we could use ofetch for this?

@felixgabler
Copy link
Contributor Author

Hi, just wanted to bump this again, would be super cool to have this at some point :) thank you for your help!

After a discussion with the Supabase Auth team, it seems @supabase/[email protected] might fix some websocket auth issues
Copy link
Collaborator

atinux commented Aug 13, 2024

From what I see the main issue is that ofetch is a wrapper of fetch so not sure if given as option for supabase client it will work

@felixgabler
Copy link
Contributor Author

I tried it a few months ago and gave the supabase client first ofetch and later ofetch.raw, as described here (options > global)).

If I remember correctly, the problem was that just ofetch does not adhere to the correct typing or already consumes the response body somehow and that ofetch.raw also had some issues. But maybe I am missing something around ofetch because I have not really worked with it too much. @larbish would you mind trying it on your end?

Copy link
Collaborator

@larbish larbish left a comment

Choose a reason for hiding this comment

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

Hey @felixgabler, sorry for the delay, I'm back from holdidays and want to merge all pending PRs.

I've just let a comment about the realtime.setAuth method that you're using. I don't understand the relation with the retry fetch.

Concerning this PR, it LGTM since using ofetch does not seem possible in this case. We can keep the retry as the default pattern and if people want be able to configure/remove it, they can create a PR.

Are you using this retry version on your project currently?

@@ -26,6 +31,7 @@ export default defineNuxtPlugin({

// Updates the session and user states through auth events
client.auth.onAuthStateChange((_, session: Session | null) => {
client.realtime.setAuth(session?.access_token ?? null)
Copy link
Collaborator

@larbish larbish Aug 29, 2024

Choose a reason for hiding this comment

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

What is this line for? I can't find any documentation about this realtime.setAuth method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this slipped in there because there is a realtime bug that I am debugging with the Supabase team where the auth token does not correctly propagate to realtime. Will take it out for now

@felixgabler
Copy link
Contributor Author

Hey @felixgabler, sorry for the delay, I'm back from holdidays and want to merge all pending PRs.

No worries, hope your holidays were great!

I've just let a comment about the realtime.setAuth method that you're using. I don't understand the relation with the retry fetch.

Concerning this PR, it LGTM since using ofetch does not seem possible in this case. We can keep the retry as the default pattern and if people want be able to configure/remove it, they can create a PR.

Sounds good!

Are you using this retry version on your project currently?

Yes, I've been using this for a while now in prod and it seems to have brought down the "Failed to fetch" errors in Sentry quite a bit.

@larbish larbish merged commit 98ca959 into nuxt-modules:main Aug 30, 2024
5 checks passed
@felixgabler felixgabler deleted the fg/retry_fetch branch August 30, 2024 19:14
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.

4 participants