-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: Forward operation in StackLink.request()
when offline
#1487
Conversation
return await this.executeQuery(operation) | ||
} catch (err) { | ||
if (isReactNativeOfflineError(err)) { | ||
return forward(operation) |
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 assume this is a fallback in case the this.isOnline()
was not defined or wrongly returned true? I see the value, but I'm wondering whether it is good to have this kind of logic here.
What if someone has a Cozy react-native app with only the StackLink defined? Here, it will forward the operation and the app will never receive the 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.
I assume this is a fallback in case the this.isOnline() was not defined or wrongly returned true?
Yes, this is mainly because when a device loose the Internet connection, it can take a few seconds before the offline status is triggered. This often happens when you are connected to an Internet box through Wifi. If you unplug the internet connection, then your computer/phones will still displayed the "online" icon for a few seconds.
What if someone has a Cozy react-native app with only the StackLink defined? Here, it will forward the operation and the app will never receive the error
Good catch. But I'm not sure that we can check if any next link
exist. I'll investigate how to do this correctly.
a0ba077
to
c87fea9
Compare
c87fea9
to
faa91a8
Compare
b8abde2
to
d5677b5
Compare
faa91a8
to
d101bfc
Compare
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.
Looks good, but I have a concern on error handling when only StackLink is used
export const isReactNativeOfflineError = err => { | ||
// This error message is specific to ReactNative | ||
// Network errors on a browser would produce another error.message | ||
return err.message === 'Network request failed' |
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 wonder how stable this message is, and if it could change in future RN upgrades...
d5677b5
to
95978d3
Compare
d101bfc
to
4bcff72
Compare
95978d3
to
1ba2e2a
Compare
On the Flagship app we want to serve `.query()` request using the `StackLink` when the device is connected, but we want to fallback to the `PouchLink` when we detect a connection loss To allow this, we allow the consuming app to provide an `isOnline()` method to the `StackLink` When provided, the `StackLink` will check for connectivity before doing its request. When offline, instead of processing the request, it will instead forward the request to the next `Link` The code is generic so the consuming app can configure a `PouchLink` or anything else as the next `Link`
On the Flagship app we want to serve `.query()` request using the `StackLink` when the device is connected, but we want to fallback to the `PouchLink` when we detect a connection loss In previous commit we tried pro-actively detect for connexion loss by calling an `isOnline()` method before processing the request But we want to also catch network errors when the `isOnline()` methods fails to detect connection loss, then we also fallback to the next `Link`
4bcff72
to
63ad248
Compare
Note
This PR follows #1483 and #1486
On the Flagship app we want to serve
.query()
request using theStackLink
when the device is connected, but we want to fallback to thePouchLink
when we detect a connection lossTo allow this, we allow the consuming app to provide an
isOnline()
method to theStackLink
When provided, the
StackLink
will check for connectivity before doing its request. When offline, instead of processing the request, it will instead forward the request to the nextLink
The code is generic so the consuming app can configure a
PouchLink
or anything else as the nextLink
We want to also catch network errors when the
isOnline()
methods fails to detect connection loss, then we also fallback to the nextLink
Related PRs: