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

dbAuth: Catch fetch failure in getToken #9119

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Sep 1, 2023

We have this comment in useToken()

/**
* Clients should always return null or token string.
* It is expected that they catch any errors internally.
* This catch is a last resort effort in case any errors are
* missed or slip through.
*/
try {
const token = await authImplementation.getToken()

And for example our Netlify auth implementation does this:

getToken: async () => {
try {
// The client refresh function only actually refreshes token
// when it's been expired. Don't panic
await netlifyIdentity.refresh()
const user = await netlifyIdentity.currentUser()
return user?.token?.access_token || null
} catch {
return null
}
},

But dbAuth let the exception slip through to useToken.
This PR adds a catch to the promise and returns null, just like Netlify (and others)

Before merging this though, I need to make sure I'm not undoing the fix @standup75 did in #8284
(@standup75 can you say off the top of your head if my code will work or not?)

@standuprey
Copy link
Contributor

finally will still get called, so lgtm, thanks for asking!

@Tobbe Tobbe added the release:fix This PR is a fix label Sep 5, 2023
@Tobbe Tobbe added this to the next-release milestone Sep 5, 2023
@Tobbe Tobbe enabled auto-merge (squash) September 5, 2023 20:02
@Tobbe Tobbe merged commit e689299 into redwoodjs:main Sep 5, 2023
29 checks passed
@Tobbe Tobbe deleted the tobbe-dbauth-catch-gettoken branch September 5, 2023 20:46
jtoar pushed a commit that referenced this pull request Sep 6, 2023
We have this comment in `useToken()`


https://github.com/redwoodjs/redwood/blob/4b734d30c6830172194a2518ae0c2cbf6f1a0904/packages/auth/src/AuthProvider/useToken.ts#L7-L14

And for example our Netlify auth implementation does this:


https://github.com/redwoodjs/redwood/blob/4b734d30c6830172194a2518ae0c2cbf6f1a0904/packages/auth-providers/netlify/web/src/netlify.ts#L68-L78

But dbAuth let the exception slip through to `useToken`.
This PR adds a `catch` to the promise and returns `null`, just like
Netlify (and others)


Before merging this though, I need to make sure I'm not undoing the fix
@standup75 did in #8284
(@standup75 can you say off the top of your head if my code will work or
not?)
@jtoar jtoar modified the milestones: next-release, v6.2.0 Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants