search

gladly-team / next-firebase-auth

Simple Firebase authentication for all Next.js rendering strategies
https://nfa-example-git-v1x-gladly-team.vercel.app/
MIT License
1.34k stars 291 forks source link

Handle fetch top-level errors #618

Closed valeriangalliat closed 1 year ago

valeriangalliat commented 1 year ago

Diff best viewed in ignore whitespaces mode.

Handle fetch top-level errors

This PR wraps the await fetch calls to login and logout API endpoints by a try/catch block so that we call onLoginRequestError and onLogoutRequestError not-only in case of HTTP errors, but also in case of network errors (e.g. fetch rejecting with TypeError: failed to fetch), instead of leaving those unhandled.

~Retry network errors~

Moved to its own PR Also in order to mitigate network errors, this PR introduces a retry logic when calling the login and logout API endpoints, provided by [fetch-retry](https://www.npmjs.com/package/fetch-retry). This module defaults to retrying up to 3 times at 1s intervals, and only retrying network errors (the case where `fetch` rejects, as opposed to HTTP errors where it resolves with `response.ok = false`). I think those defaults are sensible, and I added a configuration option `fetchRetryConfig` to allow fine tuning the retry logic if needed.

I think the rate of fetch network errors I'm seeing on login API calls is exacerbated by the fact the login API is called on every page (https://github.com/gladly-team/next-firebase-auth/issues/424 - I tried coming up with a solution for that too but didn't find a reliable way to do that just yet), nevertheless this PR shouldn't leave those errors unhandled, and thanks to the retry logic, should lower their frequency.

Let me know what you think and if you need me to make any edits!

vercel[bot] commented 1 year ago

@valeriangalliat is attempting to deploy a commit to the Gladly Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated
nfa-example βœ… Ready (Inspect) Visit Preview πŸ’¬ Add your feedback Mar 9, 2023 at 8:15PM (UTC)
kmjennison commented 1 year ago

@valeriangalliat Thanks for the PR!

The try/catch behavior makes sense and would be good to merge. Could you break that into its own PR?

I don't think we should add fetch-retry as a dependency, given that developers might prefer other fetch implementations and it'll add to bundle size. Instead, I see two alternatives:

  1. Developers should define the global fetch to have desired behavior. There was a Next.js issue to build in fetch retries, though I'm not sure what the outcome was. It's also not clear to me how easy it is to change the default fetch polyfill (related issue). If anybody has any suggestions on best practices for modifying global fetch, please chime in.
  2. This library could take a fetcher function as an input. E.g., how SWR does this: https://swr.vercel.app/docs/data-fetching#fetch
valeriangalliat commented 1 year ago

That makes sense! I like the idea of the fetcher function. I'll split the changes and make another PR for that early next week πŸ‘Œ

valeriangalliat commented 1 year ago

@kmjennison updated! This PR now only includes catching of fetch errors

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f46d5ed) 99.25% compared to head (8fb6693) 99.25%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## v1.x #618 +/- ## ======================================= Coverage 99.25% 99.25% ======================================= Files 30 30 Lines 673 675 +2 Branches 223 223 ======================================= + Hits 668 670 +2 Misses 5 5 ``` | [Impacted Files](https://codecov.io/gh/gladly-team/next-firebase-auth/pull/618?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gladly-team) | Coverage Ξ” | | |---|---|---| | [src/useFirebaseUser.js](https://codecov.io/gh/gladly-team/next-firebase-auth/pull/618?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gladly-team#diff-c3JjL3VzZUZpcmViYXNlVXNlci5qcw==) | `100.00% <100.00%> (ΓΈ)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gladly-team). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gladly-team)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.