Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add integration for offline support #2778
Add integration for offline support #2778
Changes from 12 commits
378c344
5ac1d5f
fc6462b
26dd7b0
b38a7af
f122fb5
47cbc4c
1df5a02
de32dc3
3146924
efbee73
d758d22
bd3ce31
ac720e2
83e4f07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Could
createInstance
fail? What happens tothis.offlineEventStore
then?We can import localforage typings if they exist as well.
Also, what browsers does
localforage
support? It's fine if it doesn't match Sentry's (https://docs.sentry.io/platforms/javascript/#browser-table) - we just have to document it then.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.
Ah, good catch. I meant to wrap this in a
try/catch
. I will look into importing the type file too.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 think it meets the compatibility requirements. It is a wrapper around multiple storage mechanisms. It attempts to use IndexedDB, then falls back to WebSQL, and if all else fails, it falls back to localStorage.
https://github.com/localForage/localForage/wiki/Supported-Browsers-Platforms
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.
Nice ok, makes sense. Just one last thing, what happens when local storage is full? Is it a queue, or do new events just get dropped?
@bruno-garcia, how do we handle offline queue in mobile right now?
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.
Only envelopes are cached (no event json alone are cached anymore is what I mean). The order they were cached is kept when attempting to send things to Sentry (i.e device is back online). If
maxItems
is reached (by default 30 IIRC), the oldest file is deleted to make space to the newest one.We don't use dependencies or a DB of any sort. It's just files written to a directory.
If session data is stored (release health), the
init=true
flag must be preserved. So when deleting a file to make room for a new one (max items reached), the envelope would need to be unwrapped, checked for sessions and if init=true exists, that must be moved to the next session update queued up for submission (from oldest to newest).Events capture when session tracking is on must be in an envelope together with a session update.
Note that the order being kept means that if you get a call to
captureException
, you won't be sending that to Sentry if there's stuff cached. You must prioritize what's on the storage first.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.
Thanks Bruno for coming through 🚀. We can ignore all the sessions stuff, and envelope details - although important for me, probably not as for you @voraciousdev
Basically there are two things we need to keep in mind.
Enforce some kind of
maxItems
in local storage. This can be changed through an option.Make sure that we send all events from local storage first before we continue on with other events. I know we are using an event listener right now to listen to
online
, do you think that is good enough? Maybe we need another check in theglobalEventProcessor
to be sure.As for how this will work with sessions, we can get to that when release health comes to JS 😄
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.
@AbhiPrasad the
LocalForage.setItem
promise will be rejected if there is not enough space on the device. With the way it's currently written in this integration, that would result inlogger.warn('could not cache event while offline')
being called.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.
Ok perfect we should be good then.
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.
@AbhiPrasad I'm not sure if this is directed at me, but there is definitely a possible race condition here regarding the
online
event. Given the nature of this integration (to support offline-capable apps), I think it's a real scenario that we could have real error events triggered as soon as theonline
event is fired. For offline apps, it's common practice to listen to theonline
event and run some code (which could then lead to an error being thrown/recorded).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.
Just added localforage typings as well. Good call on that. 👍
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.
Event listener should be set up after hub is set on instance (in
setupOnce
method) as event may be fired before hub is set (hub is used in_sendEvents
).IMHO it's bad practice to have business logic in constructor.
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.
Fixed in #2889