Handle fetch top-level errors

[valeriangalliat]

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.

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 (#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]

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

A member of the Team first needs to authorize it.

[vercel]

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]

@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

[kmjennison]

Please break out the try/catch changes into its own PR, excluding the addition of fetch-retry.

[valeriangalliat]

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]

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

[codecov]

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

@@           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	Coverage Δ
src/useFirebaseUser.js	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[kmjennison]

LGTM. Thank you!