-
Notifications
You must be signed in to change notification settings - Fork 487
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
fix(expo-offline-support): Add Handling network errors
section
#1806
fix(expo-offline-support): Add Handling network errors
section
#1806
Conversation
Hey, here’s your docs preview: https://clerk.com/docs/pr/1806 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple of minor suggestions, but mostly dropped a couple of questions about this PR.
@@ -71,3 +71,45 @@ To enable offline support in your Expo app, follow these steps: | |||
} | |||
``` | |||
</Steps> | |||
|
|||
## Handling network errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already an error handling section. Any reason this is not included in that doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is specific to network errors.
Until now, in non-standard browsers we used to mute these errors and Expo users could not handle them. Using this new experimental property prevents the muting and throws these network errors, so that the developers can handle them. So, it's somewhat specific to this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I see there is a mention of this above. When I reviewed I just looked at the PR and not the whole page. Maybe this information should be added to the 'How to handle network errors' section. I can definitely see some developers missing the line in the first section.
|
||
## Handling network errors | ||
|
||
To handle network errors in your Expo app, you can use the `isClerkRuntimeError` function alongside the `code` property to identify the specific error. Here's an example of how you can handle network errors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To handle network errors in your Expo app, you can use the `isClerkRuntimeError` function alongside the `code` property to identify the specific error. Here's an example of how you can handle network errors: | |
To handle network errors in your Expo app, you can use the `isClerkRuntimeError` function and check the `code` property to identify the specific error. Here's an example of how you can handle network errors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this showing a network_error
error when its for offline support? This seems counter intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clerk custom flows should throw errors if there is no internet connection, e.g. signIn.create()
. This error was muted until now and developers could not handle these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the above about adding context to this section.
|
||
## How to handle network errors | ||
|
||
To handle network errors in your Expo app, you can use the `isClerkRuntimeError()` function to check if the error is a Clerk-related error. Clerk-related errors are returned as an array of [`ClerkAPIError`](/docs/references/javascript/types/clerk-api-error) objects. These errors contain a `code` property that you can use to identify the specific error. See the following example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, what's the difference between isClerkRuntimeError and isClerkAPIResponseError
in our error handling guide, we use isClerkAPIResponseError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anagstef can confirm, but from a quick look at the code plus the name I think these are errors related to Clerk actually running in the app, versus Clerk interacting with the API. The network error would be runtime as its the app running but not being able to make a request to FAPI. ClerkAPIError
would be an error returned from FAPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh makes sense! thank you for the explanation ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is what @royanger said.
isClerkAPIResponseError
returns true
for FAPI errors, while isClerkRuntimeError
returns true for ClerkJS errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming!
@@ -74,6 +74,8 @@ To enable offline support in your Expo app, follow these steps: | |||
|
|||
## How to handle network errors | |||
|
|||
Until now, in Expo we used to mute these errors and developers could not handle them. This new experimental property prevents the muting and throws these network errors, allowing you to catch and handle them in your custom flows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary! We can remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I added this because @royanger said we should add this context, if I understood correctly. ref: #1806 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I think he meant adding the context about "Clerk custom flows should throw errors if there is no internet connection". Let me rewrite it and get back to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks really great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove that one section, and you should be good to merge! 😸💖 thank youuuu
addressed adding more context
Important
🔎 Previews:
Explanation:
This PR:
Checklist