Skip to content

Offline storage plugin #1165

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

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions plugins/offline-storage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* Offline Storage plugin
*
* Stores errors failed to get send and try to send them when
Copy link
Contributor

Choose a reason for hiding this comment

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

should failed to get send be that failed to be sent?

* Networkf comes back or on init
Copy link
Contributor

Choose a reason for hiding this comment

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

(sorry, this is a drive-by comment)

is this a typo, Networkf? should be Network, right?

*/

var offlineStorageKey = 'raven-js-offline-queue';

function offlineStoragePlugin(Raven, options) {
options = options || {};

function processOfflineQueue() {
// Let's stop here if there's no connection
if (!navigator.onLine) {
return;
}

try {
// Get the queue
var queue = JSON.parse(localStorage.getItem(offlineStorageKey)) || [];

// Store an empty queue. If processing these one fails they get back to the queue
localStorage.removeItem(offlineStorageKey);

Choose a reason for hiding this comment

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

multiple localStorage operations are not atomic. It is possible for multiple tabs to receive this queue before the removeItem. This needs to be inside a lock. See more info here:
https://medium.engineering/wait-dont-touch-that-a211832adc3a
http://balpha.de/2012/03/javascript-concurrency-and-locking-the-html5-localstorage/

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@graingert I don't believe that library handles atomic operations between multiple browser windows. It just removes oldest item when localstorage when it's full

Choose a reason for hiding this comment

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

Here is a link to a library I created to solve this problem. There are probably other libraries out there as well. Also feel free to just copy the library code into this repo if you don't want another dependency.

https://github.com/taylorhakes/localstorage-lock/

Copy link

@jasekiw jasekiw Mar 16, 2018

Choose a reason for hiding this comment

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

Storing an entire array of the logs at once can be quite a poor usage of memory. There might be a lot of logs if a device is left offline for extended periods of time. On hybrid apps there is a very small memory limit. If there are a lot of logs or large json encoded objects, memory can spike. Possibly using indexdb or an option for a custom storage adapter would allow for more memory efficient ways of storing.


queue.forEach(function processOfflinePayload(data) {
// Avoid duplication verification for offline stored
// as they may try multiple times to be processed
Raven._lastData = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I understand your comment here.
There shouldn't be any duplicates here already, as failure event is triggered at the end of _sendProcessedPayload which already checks for duplicates first.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but then when trying to send it again when get back offline, because _lastData is set with potentially the same data we will be try to send, it will be missed because it's the same data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right :)


// Try to process it again
Raven._sendProcessedPayload(data);
});
} catch (error) {
Raven._logDebug('error', 'Raven transport failed to store offline: ', error);
}
}

// Process queue on start
processOfflineQueue();

// Add event listener on onravenFailure and store error on localstorage
document.addEventListener('ravenFailure', function(event) {
if (!event.data) {
return;
}

try {
var queue = JSON.parse(localStorage.getItem(offlineStorageKey)) || [];
queue.push(event.data);
localStorage.setItem(offlineStorageKey, JSON.stringify(queue));
} catch (error) {
Raven._logDebug('error', 'Raven failed to store payload offline: ', error);
}
});

// Add event listener on online or custom event to trigger offline queue sending
window.addEventListener(options.onlineEventName || 'online', processOfflineQueue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any alternative event that I don't know of, but could be used here? :P

Copy link
Author

Choose a reason for hiding this comment

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

This is just because the application might rely on a different event for letting other plugins know when the connection is available (e.g. on our cordova-based hybrid app we trigger a "foo-bar-online" event to the document when we're sure the app is online and it was able to make an XHR request), because "online" can easily mean "connected to the network" as in "local network" but still not be connected to the internet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes perfect sense

}

module.exports = offlineStoragePlugin;