Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Strange behaviour on IE11 #47

Closed
jasalguero opened this issue Nov 12, 2015 · 16 comments · Fixed by #54
Closed

Strange behaviour on IE11 #47

jasalguero opened this issue Nov 12, 2015 · 16 comments · Fixed by #54
Labels

Comments

@jasalguero
Copy link

Hi,

I'm using the plugin to store a couple of simple values in local storage and it works all fine, except in IE (IE11), where there is no initial value set, and on changing (is a page size that I can change in an input) it gets in an infinite loop setting the variable to the new and initial value back and forth.

@fsmanuel
Copy link
Member

@jasalguero I just did a quick google for 'IE11 localStorage' and it seems that the IE11 has a lot of problems with localStorage. What is the exact version number?

@fsmanuel
Copy link
Member

@jasalguero can you try to debug the issue by removing these lines?

@varoot
Copy link

varoot commented Nov 16, 2015

I have the same issue with IE (10 and 11). Adding if (document.hidden) in the above section seems to fix the issue. This prevents the current tab from taking events fired from other (inactive) tabs, I guess.

@fsmanuel
Copy link
Member

@varoot hmm but that would make the entire purpose of the storage event obsolete, doesn't it?

@fsmanuel
Copy link
Member

@varoot wait! does your code look like:

    if (window.addEventListener && document.hidden) {
      window.addEventListener('storage', (event) => {
        if (event.key === storageKey) {
          this.set('content', JSON.parse(event.newValue));
        }
      });
    }

That would mean that the IE fires in the same tab the change occurred and that's why it results in an infinite loop. So it is the opposite of your guess but could be the fix.

@fsmanuel
Copy link
Member

we might need a check event.newValue !== event.oldValue as well and probably a check if the content !== JSON.parse(event.newValue) before doing the set on content.

@fsmanuel fsmanuel added the bug label Nov 17, 2015
@krivaten
Copy link

Just found this in our app as well. According to CanIUse "The storage event's oldValue and newValue are identical (newValue is supposed to contain the storage's updated value)". Not sure if that gives anyone aid, but trying to look in to this as well.

@varoot
Copy link

varoot commented Nov 26, 2015

@fsmanuel No, my code is like this:

if (window.addEventListener) {
    window.addEventListener('storage', (event) => {
        if (document.hidden && event.key === storageKey) {
            this.set('content', JSON.parse(event.newValue));
        }
    });
}

which means that only hidden (inactive) tabs will take values. I think it makes sense since it's probably the active tab that sends out events in the first place.

You can look at my commit here: https://github.com/smartkarma/ember-local-storage/commit/1875172126b3e8e61800582c61c0ac55f2eb9dd8

@varoot
Copy link

varoot commented Nov 26, 2015

The downside of this approach (that I can think of) is that document.hidden has less browser support than localStorage itself (compare Page Visibility API with Web Storage API)

But for me this is acceptable, since it won't break the functionality even if the browser doesn't support document.hidden, it just won't propagate the changes from one tab to another, which is a lot better than just freezing IE.

@fsmanuel
Copy link
Member

@varoot can you test if 6603619 fixes the issue?
I have no IE11 at hand to test it.

@varoot
Copy link

varoot commented Nov 26, 2015

Yeah, it works. Tested on IE10 and IE11.

@fsmanuel
Copy link
Member

I'll try to release the fix tomorrow.

@fsmanuel
Copy link
Member

@jasalguero @krivaten @varoot I released 0.1.2. Thanks for the feedback.

@MattNguyen
Copy link

This is still a problem if you call set multiple times within the same loop.

For example, we nest localStorage items for namespacing purposes, potentially calling set twice in total.

if (!storage.get('123')) { storage.set('123', {}) };
storage.set('123.john', { language: 'english' });

This in turn causes the storage change event to fire repeatedly:

// first event
console.log(event.oldValue) // => null
console.log(event.newValue) // => { '123': {} }

// next event
console.log(event.oldValue) // => { '123': {} }
console.log(event.newValue) // => { '123': { john: { language: 'english } } }

// next event
console.log(event.oldValue) // => { '123': { john: { language: 'english } } }
console.log(event.newValue) // => { '123': {} }

// next event
console.log(event.oldValue) // => { '123': {} }
console.log(event.newValue) // => { '123': { john: { language: 'english } } }

// etc....

We ended up forking the lib and adding a check document.hidden, but as was previously mentioned, there's less support for the page visibility API than the web storage API.

Any ideas for a solution? Thanks!

@fsmanuel
Copy link
Member

fsmanuel commented Jan 5, 2016

I'll investigate.

@fsmanuel
Copy link
Member

@MattNguyen I released 0.1.3.

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

Successfully merging a pull request may close this issue.

5 participants