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

fix error when removing items from storage from a second tab #55

Merged
merged 2 commits into from
Mar 10, 2020

Conversation

lemonlimemango
Copy link
Contributor

Hey Countly Team, thanks for making this great product.

I have noticed a bug in my application with the Countly library, steps to reproduce:

Description of Issue and Steps to Reproduce

  1. Have a website with Countly installed
  2. Open the website in one tab
  3. Open another tab with the same website
  4. Open up the dev console in both tabs
  5. Clear an item from localStorage on one of the tabs, like localStorage.removeItem('cly_event')
  6. Notice an error pop up on the other tab cannot read property 'length' of null

Root Cause of Issue

I traced this down to a line in the Countly SDK:

if (eventQueue.length > 0) {

I noticed that another section of the Countly SDK listens to changes on localStorage, and tries to update them when they change:

window.addEventListener('storage', function(e) {
    var key = e.key + "";
    
    switch(key) {
    //queue of requests
    case Countly.namespace + "cly_queue":
        requestQueue = Countly.deserialize(e.newValue);
        break;
    //queue of events
    case Countly.namespace + "cly_event":
        eventQueue = Countly.deserialize(e.newValue);
        break;
    case Countly.namespace + "cly_remote_configs":
        remoteConfigs = Countly.deserialize(e.newValue);
        break;
    case Countly.namespace + "cly_ignore":
        Countly.ignore_visitor = Countly.deserialize(e.newValue);
        break;
    case Countly.namespace + "cly_id":
        Countly.device_id = Countly.deserialize(e.newValue);
        break;
    default:
        // do nothing
    }
});

So, for example, if we call localStorage.removeItem('cly_event'), the above code will be run and the new value of eventQueue will be null.

Then, when eventQueue.length is called, an error will be thrown.

I discovered the issue when running Cypress tests. Cypress clears lots of state-ish things between tests, and sometimes it does that just after the application has loaded, causing this error.

From reading the code, it seems to me like you never really want these values to be null, even if they are null in localStorage.

What do you think of this PR? Also, if this can be merged, how can I get the change into the minified file? Are there any tests you need written?

Thanks!

lib/countly.js Outdated
break;
case Countly.namespace + "cly_remote_configs":
remoteConfigs = Countly.deserialize(e.newValue);
remoteConfigs = Countly.deserialize(e.newValue || {});

Choose a reason for hiding this comment

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

deserialize must receive a string to JSON.parse, right? Not actual objects or arrays, I think.

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @mt-ronkorving ! Updated.

Anything else that you think needs to be added here? Anything I need to do get the minified JS updated? cc @ar2rsawseen

Thanks again team!

@ar2rsawseen ar2rsawseen merged commit fd92cc3 into Countly:master Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants