Skip to content
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

Why records property is not updating with fetch? #177

Open
DmitryFillo opened this issue May 9, 2015 · 14 comments
Open

Why records property is not updating with fetch? #177

DmitryFillo opened this issue May 9, 2015 · 14 comments
Labels

Comments

@DmitryFillo
Copy link

I use localStorage collection, works great. But I want use storage event for updating collection in many tabs. Idea: do fetch() when storage event fires.

bindStorageEvent : function() {
    var _this = this;
    $(window).on('storage.'+this.localStorage.name, function(e) {
        if(e.originalEvent.key === _this.localStorage.name) {
            _this.fetch();
        }
    });
},

unbindStorageEvent : function() {
    $(window).off('storage.'+this.localStorage.name);
}

But this shouldn't work as expetcted because records property will not be updated with fetch and save() method use records prop. And so all actual data which came from one tab will be rewrited by old data from another tab.

So I rewrited code in this way:

bindStorageEvent : function() {
    var _this = this;
    $(window).on('storage.'+this.localStorage.name, function(e) {
        if(e.originalEvent.key === _this.localStorage.name) {

            // the dirtiest records update way, only for test
            var store = _this.localStorage.localStorage().getItem(_this.localStorage.name);
            _this.localStorage.records = (store && store.split(",")) || [];

            _this.fetch();
        }
    });
},

unbindStorageEvent : function() {
    $(window).off('storage.'+this.localStorage.name);
}

And it works great.

Suggested solution: I think that we can put this code to the another method in the localStorage object (records_update) and call this method from constructor and from Backbone.sync() when doing fetch() or from findAll() before getting particular model. I think this property must be updated on fetch, not only at the collection initialization time.

@scott-w
Copy link
Collaborator

scott-w commented Mar 4, 2017

So if I'm understanding it correctly, every time you run fetch(), the in-memory model should update with whatever's in the localStorage, overwriting the data in the model/collection. I'll pull a test together to see if I can verify this behaviour.

@scott-w scott-w added the bug label Mar 4, 2017
@scott-w scott-w added this to the 2.0 milestone Mar 4, 2017
@scott-w scott-w removed this from the 2.0 milestone Mar 25, 2017
@scott-w
Copy link
Collaborator

scott-w commented Mar 25, 2017

I'm removing the 2.0 milestone until I can understand what needs to happen here. I'm thinking it might be a bug but I'm not sure.

@ChrisHarrow
Copy link

I have the same problem.
If I replace "this.records" in the function "findAll" with "localStorage.getItem(this.name).split(',')" everything is working.
model.LocalStorage and the browser localStorage seem to be out of sync.

@scott-w
Copy link
Collaborator

scott-w commented Jun 26, 2017

Hi @chris136 - is the local storage being updated in multiple places? e.g. are there two models referencing the local storage record?

@ChrisHarrow
Copy link

No, just one model. I use it to save calendar events created via drag and drop. I noticed newly created events were missing when I switched a week back and forth. But after a complete site reload, the events were back where they were created.
After this little fix I mentioned before, everything worked as intended.

@scott-w
Copy link
Collaborator

scott-w commented Jul 4, 2017

Thanks for the information @chris136 If I understand this correctly, what's happening is:

  1. Fetch all events from localStorage
  2. Add new event
  3. Move away from view
  4. Move back to view
  5. Step 1. fires again but the newly added item in localStorage isn't in your view

Basically, once there's a list of records in the collection, fetch is just returning that list instead of checking the localStorage again.

I can have a look at this over the weekend and try to re-create the issue in a test environment.

scott-w pushed a commit to scott-w/Backbone.localStorage that referenced this issue Jul 22, 2017
I'm not able to test the reaction to the storage event as it only fires on the window that's _not_ doing the `setItem` call.
@scott-w
Copy link
Collaborator

scott-w commented Jul 22, 2017

Hi @chris136

Would it be possible for you to check the version of Backbone.localStorage that you're using? I've been trying to recreate this in tests and the output seems to be acting the way I'd expect in version 2.0.0.

@ChrisHarrow
Copy link

@scott-w according to the changelog I use version 1.1.16 (January 22, 2015).
I installed it via NPM @2.x and it said version 2.0.0

@scott-w
Copy link
Collaborator

scott-w commented Jul 23, 2017

Thanks @chris136, is the issue still present?

@ChrisHarrow
Copy link

@scott-w yes, that is the version where I noticed the problem.

@ChrisHarrow
Copy link

I try to make a minimal test case if I find the time this week.

@scott-w
Copy link
Collaborator

scott-w commented Jul 23, 2017

HI @chris136

That'll be awesome, thanks 👍

scott-w added a commit that referenced this issue Aug 21, 2017
This commit fixes the RequireJS build and adds some tests related to the local storage issues reported a while back. I've bumped the package version as part of this.

* Added rules for fixing lint errors
* Added tests for #177
* Updated builds to fix requirejs #220

Close #220
@yahuarkuntur
Copy link

Any updates on this issue? or is there any workaround?

Cheers!

@blikblum
Copy link

I fixed this in my backbone fork.

Look at the following commits:
blikblum/nextbone@8054cdd
blikblum/nextbone@e183cce

Should not be hard to make a PR. Currently i'm on a slow internet connection so cannot do it myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants