Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Offline status codes #116

Open
stevebartholomew opened this issue Jun 27, 2014 · 13 comments
Open

Offline status codes #116

stevebartholomew opened this issue Jun 27, 2014 · 13 comments
Assignees
Milestone

Comments

@stevebartholomew
Copy link

Is there a reason that offline status codes are limited to 502 & 408?

We're having issues where locally stored data is not marked as dirty even though the request never reached the server. Looking at this list I wonder if it's because the response is something other than 502 & 408.

Before checking I assumed it was checking for success responses (i.e. 2xx) and keeping the data if not.

Is there a reason it's done the other way round?

@nilbus
Copy link
Owner

nilbus commented Jun 27, 2014

I am open to other suggestions, as long as they indicate directly that the server is offline and not a status that could indicate other problems. For example, a 403 (permission denied), 404 (not found), 500 (application error) are errors that indicate a different problem than being offline in most cases but can in some. However we don't ever want to cache as if the server was offline when the retrying the error will never help. For example, if a API URL changed or there is a 500 application error, we want to raise the error rather than caching for retry on sync. Does that make sense to you?

@stevebartholomew
Copy link
Author

After speaking to a couple of other devs I do understand your reasoning. I think we're probably hitting a slightly different use case. I've changed things so that everything saves locally and syncs later by reading _dirty records directly.

However I've noticed that once the records are in local storage, failed saves (i.e. anything other than success) will not remove the ids from the _dirty list. This is actually very useful for me but I'm not sure it's intended functionality.

I'll do some more digging next week and let you know if there's anything odd going on.

I would also note that other errors are not always a sign of a permanent problem and I'd not be happy to potentially lose important data because of them. Now you have ajax errors firing the correct handlers those things can be caught by your error tracking.

I would way rather have unsync'd data on the users side than risk losing anything!

@elad
Copy link
Contributor

elad commented Jun 29, 2014

The offline status codes aren't limited, you can change them - see "Offline state detection" in the README.

I'm not sure what you're trying to do, but I believe if you end up reading dirty records from localStorage directly you might want to reconsider because that might be very difficult to maintain. (Think opacity violation.)

@stevebartholomew
Copy link
Author

Oh I think I'm definitely misusing it at this point. A 'store and queue' method is much more suitable for our use case but it kinda evolved that way. Reading of _dirty is so we can build a manifest of records to re-save.

Anyway, I hadn't spotted the offline state detection before but it's definitely more appropriate for us to store unless we get a success from the server. It's data that we definitely can't afford losing.

I think my original question is invalid now really so I'll close. You're doing the right thing and allowing folks to add their own means it's flexible. Given our use case we'll probably roll our own or fork the code to fit - which will save you from our weird requests :)

@nilbus
Copy link
Owner

nilbus commented Jun 30, 2014

Rather than forking, you can use an offlineStatusCodes method like this, which will report every non-200 status as offline:

# Consider any non-success code to be offline
Backbone.DualStorage.offlineStatusCodes = function(xhr) {
    return (xhr.status === 200) ? [] : [xhr.status];
}

I am considering adding this example to our Readme.

On a related note, would it be more intuitive to have an offline method that returns true/false than an offlineStatusCodes method that returns an array? Either way you will have access to the xhr object, but if you rely on things other than response status, then you won't have do awkward things like have a conditional that returns the response's status to go offline. /cc @elad

@nilbus nilbus reopened this Jun 30, 2014
@nilbus nilbus self-assigned this Jun 30, 2014
@elad
Copy link
Contributor

elad commented Jun 30, 2014

@nilbus - that was my intuition, that an array makes sense for default and initialization and a function makes sense when it returns a boolean. But like I said at the time, I don't have strong feelings either way.

@stevebartholomew - the main concern here isn't misuse, but that persistent storage might change from localStorage to say IndexedDB and then your code will break. Maybe the takeaway here is that once storage is abstracted we should provide an API to the storage layer itself, but that's @nilbus's decision and personally I'd like to hear more about the use case first.

@stevebartholomew
Copy link
Author

We're only reading from _dirty so we can pick up what's not yet been sync'd. I'd rather we didn't have to of course.

The two reasons for this are:

  • Determine what needs to be re-saved now we're (probably) online
  • Merge any local data with remote data if the remote pulls happen before the remote pushes (async -yeay!)

I've not mentioned this yet but this is on a mobile app using Cordova - so offline/flaky connections are happening all the time.

@elad
Copy link
Contributor

elad commented Jun 30, 2014

Can't you use dirtyModels() for that instead of directly accessing localStorage?

@nilbus
Copy link
Owner

nilbus commented Jun 30, 2014

Have you looked at using syncDirtyAndDestroyed after returning online for
syncing things that were saved while offline (dirty)?

@stevebartholomew
Copy link
Author

Trawling through the code I'm really not sure why we're not using dirtyModels().

We do some determination about what goes into that sync list, based on attributes of the items, but we could do that by loading up from the list returned. For that reason to sync everything via syncDirtyAndDestroyed is not always appropriate.

That said, you're making question some of the way we're using this stuff so I'm going to chat with other devs here and see if we can simplify our lives a bit :)

@nilbus
Copy link
Owner

nilbus commented Jun 30, 2014

One of my near goals is to document our API better, beyond just having a
Readme.

@stevebartholomew
Copy link
Author

Well you do have these things there and we did read the code quite a bit to understand what's going on. I have a feeling that was focused on the sync overriding rather than the other utility methods though :)

@nilbus nilbus modified the milestone: 2.0 Jul 7, 2014
@nilbus
Copy link
Owner

nilbus commented May 3, 2015

Action summary: Transition from using Backbone.DualStorage.offlineStatusCodes to something like Backbone.DualStorage.isOffline.

@nilbus nilbus modified the milestones: 2.0, 2.1 May 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants