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

Added option to allow localStorage to update first and return successful immediately. #86

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

maxfi
Copy link

@maxfi maxfi commented Jan 21, 2014

Remote sync is then achieved by calling syncDirtyAndDestroyed().

…ful immediately.

Remote sync is then achieved by calling `syncDirtyAndDestroyed()`.
@@ -27,6 +28,7 @@ Backbone.Collection.prototype.syncDestroyed = ->
for id in ids
model = new @model(id: id)
model.collection = @
model.remoteFirst = true if (result(model, 'localFirst') or result(model.collection, 'localFirst'))
Copy link
Contributor

Choose a reason for hiding this comment

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

model.remoteFirst = (result(model, 'localFirst') or result(model.collection, 'localFirst')) is better?

Copy link
Owner

Choose a reason for hiding this comment

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

Or this:

model.remoteFirst = result(model, 'localFirst') or result(model.collection, 'localFirst')

There are lots of ways to skin a cat. I don't have a strong preference in this case.

@tute
Copy link
Contributor

tute commented Jan 21, 2014

So localFirst is different from local in that localFirst queues up the changes for syncing later, and not local?

Would you like to follow the approach in #65 (comment), in which a local fetch happens instantly, and then fires out a connection, so as to be as snappy as it can be, but be in sync with the server whenever it can?

@maxfi
Copy link
Author

maxfi commented Jan 21, 2014

localFirst is pretty much the same as local except that it marks records with _dirty when making changes. Then, when syncDirtyAndDestroyed() is called it works the same as dualSync worked before, which I have abstracted out to remoteSyncFirst(). So, at the moment, my app is doing it's own checks every 10 seconds to see if the client has an acceptable internet connection, and if it does, then syncDirtyAndDestroyed() is called. Ideally syncDirtyAndDestroyed() should have a callback or some built-in protection that avoids it being called whilst a sync is in process. That's what I'm trying to implement now.

@@ -16,6 +16,7 @@ Backbone.Collection.prototype.syncDirty = ->

for id in ids
model = if id.length == 36 then @findWhere(id: id) else @get(id)
model.remoteFirst = true if (result(model, 'localFirst') or result(model.collection, 'localFirst'))
model?.save()
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of setting an attribute on the model, what do you think about passing in a special option to save, such as save(remoteFirst: true)? It should get passed through to sync/dualStorage. Then you also won't have to clean it up later.

@nilbus
Copy link
Owner

nilbus commented Jan 30, 2014

I like the direction you're going with this. Sorry I couldn't get to reviewing it until now. I want to give some thought to the names localSyncFirst and remoteSyncFirst. Maybe it's fine though. Go ahead and clean up based on the comments.

@ghost ghost assigned nilbus Jan 30, 2014
@nilbus
Copy link
Owner

nilbus commented Jan 31, 2014

This will need to have unit tests before we can pull it in.

@maxfi
Copy link
Author

maxfi commented Feb 19, 2014

Apologies about the delay in response.

I've just pushed some more changes to my forked repo.

Goals in this version:

  • Instantaneous sync (to/from localStorage) on slow/offline connections (think EDGE and 3G).
    • Prefer localStorage for sync.
  • Background sync of localStorage to server.
    • Uses a request queue to make sure that requests arrive at the server in the correct order.
    • Removes previous fetch requests from the queue if a new fetch request is issued.
    • If the application is closed whilst the request queue still has requests it should be re-populated when syncDirtyAndDestroyed() is called on the model/collection.
  • Collections/models in localStorage that are !isNew() should be removed from localStorage and in-memory collections/models if they do not exist on the server (i.e., they have been removed server-side).
  • Obviously localStorage, in-memory collections/models, and server state should remain in sync at all times, and recover from out-of-sync states.
  • All normal Backbone operations should work as expected, i.e., as if Backbone was used by itself without any plugins.

From my manual testing everything seems to work OK. Unfortunately I'm not quite sure how to go about setting up the unit/integration tests to fit with the existing tests. If anyone is able to help out with this, even just to point me in the right direction, that would be greatly appreciated.

Also, I will need to refactor some of the code to be able to merge. There is also some code duplication that will need to be refactored.

Please let me know if I have overlooked anything or you have any suggestions on how to improve this push.

@tute
Copy link
Contributor

tute commented Feb 21, 2014

Is this very much different than the approach in linked comment? #65 (comment) With current nilbus/Backbone.dualStorage master that code makes it work local first, and if online it syncs after. Only weird thing is the two events being fired, for local and remote saves.

Also, for slow connections I added following snippet, which cancels "long" running connections, please let me know what you think:

# A phone with Sim card but no plan thinks it has connection, same situation
# as with very slow connections. This makes jQuery less tolerant with slow
# connections, raises a connection error effectively switching dualStorage
# to offline.
$.ajaxSetup
  timeout: 2000

That way the app configures itself according to the type of connection it has.

@maxfi
Copy link
Author

maxfi commented Feb 21, 2014

@tute Yes, this is the same approach in that a local sync will occur first and then the online sync is added to the request queue. Have you had a chance to try it out?

Thanks for the Ajax timeout settings. I'm actually using that in my app already but it might be nice to integrate it as an option for the request queue.

@tute
Copy link
Contributor

tute commented Feb 21, 2014

I didn't try the fork, @maxfi, but if we can mimic similar to same behavior with 5 lines of code it seems like the option to implement/iterate. Thanks for your input!

@maxfi
Copy link
Author

maxfi commented Feb 21, 2014

Haha. I agree @tute! I'll have to checkout your code and see if it does what I'm looking for.

@nilbus
Copy link
Owner

nilbus commented May 10, 2014

I haven't looked at the localSyncFirst implementation in great detail yet. However I won't be able to accept this as-is because of the massive code duplication between localSyncFirst and remoteSyncFirst. It would be quite difficult to maintain. I'll keep this around though, as it might have some potential after some refactoring love.

@nilbus nilbus removed their assignment May 10, 2014
@nilbus nilbus added the Idea label Jul 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants