-
Notifications
You must be signed in to change notification settings - Fork 927
Retry all non-Firestore exceptions #2288
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
Conversation
Our users are still seeing IndexedDB errors on iOS 13 (using IONIC/Cordova). One of the possible reasons is that some environments might not throw DOMExceptions, which would lead us to not retry. This PR flips our logic around and only skips retries if the underlying exception is a non-Firestore exception. Fixes #2232
// TODO(schmidt-sebastian): We could probably be smarter about this and | ||
// not retry exceptions that are likely unrecoverable (such as quota | ||
// exceeded errors). | ||
const retryable = | ||
idempotent && | ||
isDomException(e) && | ||
error.name !== 'FirebaseError' && |
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.
Note that the instanceof check doesn't work here since the Async/Await wraps our FirestoreError in a generic Error.
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 might be worthwhile to add as an actual comment to the code rather than just the PR.
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.
Done
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.
LGTM
Our users are still seeing IndexedDB errors on iOS 13 (using IONIC/Cordova). One of the possible reasons is that some environments might not throw DOMExceptions, which would lead us to not retry. This PR flips our logic around and only skips retries if the underlying exception is a non-Firestore exception.
Fixes #2232