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

feat: Forward operation in StackLink.request() when offline #1487

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

Ldoppea
Copy link
Member

@Ldoppea Ldoppea commented May 31, 2024

Note

This PR follows #1483 and #1486

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

We want to also catch network errors when the isOnline() methods fails to detect connection loss, then we also fallback to the next Link


Related PRs:

packages/cozy-client/src/CozyLink.js Outdated Show resolved Hide resolved
packages/cozy-client/src/StackLink.js Show resolved Hide resolved
return await this.executeQuery(operation)
} catch (err) {
if (isReactNativeOfflineError(err)) {
return forward(operation)
Copy link
Contributor

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

Copy link
Member Author

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.

@Ldoppea Ldoppea force-pushed the feat/handle_offline branch 3 times, most recently from a0ba077 to c87fea9 Compare July 17, 2024 13:34
@Ldoppea Ldoppea force-pushed the feat/handle_offline branch from c87fea9 to faa91a8 Compare July 26, 2024 14:23
@Ldoppea Ldoppea changed the base branch from master to feat/handle_virtual_documents_persistance July 26, 2024 14:27
@Ldoppea Ldoppea marked this pull request as ready for review July 26, 2024 14:46
@Ldoppea Ldoppea requested a review from Merkur39 as a code owner July 26, 2024 14:46
@Ldoppea Ldoppea force-pushed the feat/handle_virtual_documents_persistance branch from b8abde2 to d5677b5 Compare July 26, 2024 15:04
@Ldoppea Ldoppea force-pushed the feat/handle_offline branch from faa91a8 to d101bfc Compare July 26, 2024 15:04
Copy link
Contributor

@paultranvan paultranvan left a 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'
Copy link
Contributor

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...

@Ldoppea Ldoppea force-pushed the feat/handle_virtual_documents_persistance branch from d5677b5 to 95978d3 Compare August 13, 2024 15:31
@Ldoppea Ldoppea force-pushed the feat/handle_offline branch from d101bfc to 4bcff72 Compare August 13, 2024 15:31
@Ldoppea Ldoppea force-pushed the feat/handle_virtual_documents_persistance branch from 95978d3 to 1ba2e2a Compare September 9, 2024 10:02
Base automatically changed from feat/handle_virtual_documents_persistance to feat/meta_offline September 9, 2024 10:04
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`
@Ldoppea Ldoppea force-pushed the feat/handle_offline branch from 4bcff72 to 63ad248 Compare September 9, 2024 10:05
@Ldoppea Ldoppea merged commit 7dc90ba into feat/meta_offline Sep 9, 2024
2 checks passed
@Ldoppea Ldoppea deleted the feat/handle_offline branch September 9, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants