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: add offline mode #644

Merged
merged 11 commits into from
Jan 24, 2024
Merged

feat: add offline mode #644

merged 11 commits into from
Jan 24, 2024

Conversation

Mercy811
Copy link
Contributor

@Mercy811 Mercy811 commented Jan 5, 2024

Summary

Add support for offline. When online, it behaves exactly the same as previous versions. When offline, tracking an event will not schedule a flush not will only be stored in storage. When back to online, the SDK will flush all stored events.

Offline feature is support by default

To opt of the default offline logic provided by network checker plugin

amplitude.init('API_KEY', {
  offline: OfflineDisabled
});

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@Mercy811 Mercy811 marked this pull request as ready for review January 8, 2024 17:18
@Mercy811 Mercy811 force-pushed the AMP-91139-offline-mode branch from d923619 to 0f9e49a Compare January 13, 2024 00:28
@Mercy811 Mercy811 force-pushed the AMP-91139-offline-mode branch from 0f9e49a to 8d14bfc Compare January 13, 2024 00:41
@Mercy811
Copy link
Contributor Author

Mercy811 commented Jan 13, 2024

It requires setOffline() to enable custom offline logic by first opting out the default plugin and then toggling config.offline which is unlike Kotlin which has direct access to amplitude.config. I think adding setOffline() is an extended feature and we should release this version without supporting custom offline logic first. If there is customer request, then we can plan for it later.

Copy link
Contributor

@justin-fiedler justin-fiedler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Thanks @Mercy811. Really appreciate the test coverage.

See question about scheduling flush earlier on return to online.

add debug messages
@Mercy811 Mercy811 merged commit f2cd717 into main Jan 24, 2024
3 checks passed
@Mercy811 Mercy811 deleted the AMP-91139-offline-mode branch January 24, 2024 20:29
@Multiply
Copy link

It requires setOffline() to enable custom offline logic by first opting out the default plugin and then toggling config.offline which is unlike Kotlin which has direct access to amplitude.config. I think adding setOffline() is an extended feature and we should release this version without supporting custom offline logic first. If there is customer request, then we can plan for it later.

We just noticed this landing, and we'll most definitely use this functionality to pause sending events, and just let them queue up.
For the most part, we'll just do the same as the default plugin, but also check if we've paused sending events before setting it back to online.

@Mercy811
Copy link
Contributor Author

Hi @Multiply, thanks for choosing Amplitude and using the new feature! I will put it into our roadmap. Could you give me more context on the difference between the default networkConnectivityCheckerPlugin and the one you want to implement? We can see if that's what we can make it as default to support more customers with the same needs.

@Multiply
Copy link

Multiply commented Jan 25, 2024

Currently, I'm trying to set offline to disabled, and then set it to true via our own plugin, but it seems to result in the default plugin being loaded anyway. (Our plugin is most likely added first, and setup also called first, which is why the default one is still added) - Might be better to have a separate flag for disabling the default plugin?

What we're trying to achieve is storing all events in the storage provider, until a user gives consent, instead of simply dropping events.
If the user eventually consents to tracking, we'd like to push the whole queue at that point in time.

Opting out doesn't quite do what we want, but the offline variable would.

Essentially we just want to pause sending for a while.

Edit: I succeeded in disabling the plugin, by moving my initialization into the async execute call for the first event, so I could set offline to true, until consent comes in.

@Mercy811
Copy link
Contributor Author

Hi @Multiply,

What we're trying to achieve is storing all events in the storage provider, until a user gives consent, instead of simply dropping events.
That sounds like another feature request. But glad to hear that we are unblocked now.

@cgibson-emesent
Copy link

Is there going to be react-native support for offline mode when the network drops out and automatically start sending once the connection returns? I would have expected that if the attempt to send the event fails due to a network error, it's not removed from the _unset array in the local storage. I'm a bit puzzled as to what the purpose of the offline config flag is as you can then never send events.

@Mercy811
Copy link
Contributor Author

Hi @Multiply, I created a separate issue to track requests on setOffline(). Could you leave a comment with why you need this feature there? Thanks.

@Mercy811
Copy link
Contributor Author

Hi @cgibson-emesent, I also created a separate issue to track requests on supporting offline mode for React Native SDK. Please leave a comment there to help us better track customer requests.

@yuhao900914 yuhao900914 mentioned this pull request Apr 9, 2024
1 task
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.

5 participants