feat: Forward operation in StackLink.request() when offline#1487
feat: Forward operation in StackLink.request() when offline#1487Ldoppea merged 3 commits intofeat/meta_offlinefrom
StackLink.request() when offline#1487Conversation
| return await this.executeQuery(operation) | ||
| } catch (err) { | ||
| if (isReactNativeOfflineError(err)) { | ||
| return forward(operation) |
There was a problem hiding this comment.
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.
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
paultranvan
left a comment
There was a problem hiding this comment.
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.
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 theStackLinkwhen the device is connected, but we want to fallback to thePouchLinkwhen we detect a connection lossTo allow this, we allow the consuming app to provide an
isOnline()method to theStackLinkWhen provided, the
StackLinkwill check for connectivity before doing its request. When offline, instead of processing the request, it will instead forward the request to the nextLinkThe code is generic so the consuming app can configure a
PouchLinkor anything else as the nextLinkWe want to also catch network errors when the
isOnline()methods fails to detect connection loss, then we also fallback to the nextLinkRelated PRs: