Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use error method when no local store exists after remote read fails #91 #94

Merged
merged 12 commits into from

2 participants

@davetayls

This should fix the following scenario as described in #91:

  • if you make a request with default settings
  • and there is no local store created because it's the first time you have made a request
  • dualStorage will make a request to the server
  • at that point if the server fails and there is no local store would it still resolve as success?
@nilbus
Owner

This looks good so far. Could you add new tests for this behavior in spec/dualsync_spec.coffee? Please cover these 3 offline scenarios:

  • there is data in the store (call success)
  • there was never data in the store (call error)
  • the server previously returned an empty collection (call success)

I'm not convinced that the last scenario will call success like it should. I'm not sure if an empty store string will be saved (with the json []). If so, it'll work. If it stores an empty string, it will not work, because an empty string is falsy in javascript. If it stores an empty string you can use != null (cs), which will translate to !== null (js) and do the right thing for an empty string. If an empty collection response does not add an entry to the store at all, then we will need to make a change to force it to create an empty entry.

Let me know if you get stuck with the tests, and I can try to find time to work with you later in the week.

@davetayls

true, I'll get the tests in and update the pull request

@nilbus
Owner

Thanks!

@davetayls

I've done test for case 2 which is fine.

I'm now trying to write case 1 by simulating a success first then an error but the first localsync isn't actually saving any data in the test and so the second is thinking it's an empty store. can take a quick look at:

"success if server errors and Store exists but empty data" in dualsync_spec line 199

spec/dualsync_spec.coffee
@@ -180,6 +180,43 @@ describe 'delegating to localsync and backboneSync, and calling the model callba
describe 'server response', ->
describe 'on read', ->
describe 'for models', ->
+ it 'errors if server errors and no existing local Store is found', ->
+ spyOnLocalsync()
+ backboneSync.reset()
+ localsync.reset()
+ ready = false
+ runs ->
+ dualsync('read', model,
+ error: (-> ready = true)
+ serverResponse: {side: 'left', _id: 13}
+ serverResponseCode: 500
+ )
+ waitsFor (-> ready), "The error callback should have been called", 100
+ runs ->
+ expect(backboneSync.calls[0].args[0]).toEqual 'read'
+ expect(backboneSync.calls[0].args[2].storeExists).toEqual false
@nilbus Owner
nilbus added a note

Instead of testing that backboneSync gets called with storeExists in the options (which happens to be how we accomplish the goal), instead only assert that the actual goal is accomplished: the error callback is called. ready is only set to true if the error callback is called, so just the fact that waitsFor (-> ready) doesn't fail is good enough. The following runs -> function can be removed.

In other tests where I was trying to ensure that backboneSync gets called, I do make these assertions, because that's the goal of the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nilbus
Owner

I'm now trying to write case 1 by simulating a success first then an error but the first localsync isn't actually saving any data in the test and so the second is thinking it's an empty store.

You're right that it won't persist data between calls. Instead of doing two requests, at the beginning of the tests for cases 1 and 3, manually manipulate localStorage to simulate the scenario you're trying to test. For example, if you're trying to simulate there being data in the store:

localStorage.setItem(storeNameForTestModel, "1,2,3")

Then with this scenario set up for where there is data in the store, test that the success callback is called like we expect.

@davetayls

sorry i've taken so long on this, i should have these done in the next few days

@davetayls

hey @nilbus I've updated the tests and rebased on to the current master so should just fast-forward if you're happy.

@nilbus
Owner
@nilbus
Owner

Sorry, I should have thought to comment on this issue as we worked on #104 and the ticket it references. Just before you finished this up, I merged #104, which causes the error callback to be called any time the server returns an error response, rather than always calling the success callback. Now the success callback is only used on success and when truly offline. Does this change resolve the problem you were originally experiencing in #91? I'm going to close this for now because I think it does, but let me know.

Thanks for putting the effort into helping resolve this! Knowing there was more than one person experiencing this issue helped make it more obvious that the behavior needed to change.

@nilbus nilbus closed this
added some commits
@nilbus Merge branch 'options.dirty' from eladxxx
Closes #105
d4f3784
@nilbus Release version 1.3.0 7148419
@nilbus Bump version number in amd version b52e07f
@nilbus Use underscore call style compatible with lodash
Fixes #99
ba86c80
@nilbus Update Changelog for #99 8e703f6
@nilbus Use _.result instead of reimplementing it
Fixes #107
cd344f4
@nilbus Test against backbone-1.1.2 bea0707
@nilbus Fix syncDestroyed with custom idAttribute and backbone >= 1.1.1
syncDestroyed did not set the idAttribute attribute and only set
model.id. This worked in earlier versions of backbone, but it does not
as of 1.1.1. isNew now checks for @has(@idAttribute) instead of
model.id, as of
jashkenas/backbone@11cc0e8.

Also, to avoid idAttribute issues in the future, stop setting model.id
anywhere. Instead, just set the idAttribute attribute, and let backbone
set model.id.
ba5ffe7
@davetayls

I've taken a look through and it resolves with success even if there isn't a previous item stored

@nilbus
Owner

What is the response status code being returned by the server? Now we depend on whether the Ajax response is success or error to decide which callback to use.

@nilbus
Owner

Or are you talking about when the browser is offline?

@davetayls

i've followed the code through, the server responds with 0 and the code thinks it's in offline mode but then runs what we had before not considering whether there is a local store. I'm just adding the Store.exist updates i made in to the current code and will submit a new pull request with fixed unit tests

@nilbus
Owner

Right. I forgot about the offline case where there is no data. I reopened this issue. If you update your master branch, this pull request will update; no need to open a new one.

@nilbus nilbus reopened this
@davetayls

how does that look :)

@nilbus nilbus commented on the diff
backbone.dualstorage.coffee
@@ -249,14 +243,15 @@ onlineSync = (method, model, options) ->
dualsync = (method, model, options) ->
options.storeName = getStoreName(model.collection, model)
+ options.storeExists = Store.exists(options.storeName)
@nilbus Owner
nilbus added a note

For my understanding, did you make this an attribute on options rather than a local variable because you check to see if storeExists in your app's error callback to determine why it failed?

I don't but that was the thought for the potential of using it within callback inside a main app

@nilbus Owner
nilbus added a note

:thumbsup:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davetayls

not sure where those other diffs have come from, i don't get them locally. do you think you'll merge this in? cheers

@nilbus
Owner

They're commits from the most recent release. Generally those won't show up in a pull request, but maybe it's a github bug or "feature" related to the fact that this PR was closed while they happened and then reopened. I'm able to ignore them, and it won't cause any issue.

@nilbus nilbus merged commit 6cfe823 into nilbus:master
@nilbus nilbus referenced this pull request from a commit
@nilbus Update Changelog for #94 d63ec2d
@nilbus nilbus referenced this pull request from a commit
@nilbus Merge Pull Request #94 from davetayls a1178a5
@nilbus
Owner

I shuffled the tests around a bit, but this is great. Thank you!

@nilbus nilbus referenced this pull request from a commit
@nilbus Allow records to be created when offline and no store exists
This bug was introduced in #94 and v1.3.1.
fde70e1
@nilbus
Owner

@davetayls FYI, I committed fde70e1 because the change here causes all operations including save and update to error when offline with no store.

I wanted to get your opinion on what should happen on a destroy call when offline with no store. What would you expect to happen? The record may exist on the server but just not in localstorage, so my first thought would be to allow it to succeed and add the id to the destroyed list. Thoughts?

@davetayls

Sorry, I've not got back to you sooner. Ah yes, because that function gets called from other methods.

I think that makes sense to add the id to the destroyed list :+1:

@nilbus nilbus referenced this pull request from a commit
@nilbus Use the appropriate callback when using offline storage
Blanket success was incorrect. For example, when fetching and id that was never cached locally,
the error callback should be used.

Related: #94
75417f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 10, 2014
  1. Release version 1.3.0

    authored
  2. Update Changelog for #99

    authored
  3. Test against backbone-1.1.2

    authored
  4. Fix syncDestroyed with custom idAttribute and backbone >= 1.1.1

    authored
    syncDestroyed did not set the idAttribute attribute and only set
    model.id. This worked in earlier versions of backbone, but it does not
    as of 1.1.1. isNew now checks for @has(@idAttribute) instead of
    model.id, as of
    jashkenas/backbone@11cc0e8.
    
    Also, to avoid idAttribute issues in the future, stop setting model.id
    anywhere. Instead, just set the idAttribute attribute, and let backbone
    set model.id.
Commits on May 12, 2014
  1. @davetayls

    use error method when read fails

    davetayls authored
    check for null in Store.exists
  2. @davetayls
  3. @davetayls
  4. @davetayls
Something went wrong with that request. Please try again.