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

Keys & length don't take the prefix into account in localStorage #275

Closed
ocombe opened this issue Oct 16, 2014 · 3 comments · Fixed by #276
Closed

Keys & length don't take the prefix into account in localStorage #275

ocombe opened this issue Oct 16, 2014 · 3 comments · Fixed by #276

Comments

@ocombe
Copy link
Contributor

ocombe commented Oct 16, 2014

Unless I'm mistaken, the functions keys and length don't really return the correct set of values:
The keys function:

function keys(callback) {
    var self = this;
    var promise = new Promise(function(resolve, reject) {
        self.ready().then(function() {
            var dbInfo = self._dbInfo;
            var length = localStorage.length;
            var keys = [];
            for (var i = 0; i < length; i++) {
                keys.push(localStorage.key(i)
                    .substring(dbInfo.keyPrefix.length));
            }
            resolve(keys);
        }).catch(reject);
    });
    executeCallback(promise, callback);
    return promise;
}

It returns all the keys, it removes the prefix but doesn't check if there is a prefix on this key.
If you have any other value in localstorage added by an external lib, the key will be returned and will be truncated.

The length function:

function length(callback) {
    var self = this;
    var promise = new Promise(function(resolve, reject) {
        self.ready().then(function() {
            var result = localStorage.length;
            resolve(result);
        }).catch(reject);
    });
    executeCallback(promise, callback);
    return promise;
}

It will return the total number of keys in localstorage, even if some are added by external libs, we should check for the prefix here too.

@tofumatt
Copy link
Member

The clear() method actually does the right thing in this regard, so I guess we should copy its looping-of-all-keys-and-checking-for-prefix code. Obviously MUCH slower, but then localStorage is our "bad driver" anyway...

@tofumatt tofumatt added this to the 1.1 getItems(); keys() milestone Oct 16, 2014
@ocombe
Copy link
Contributor Author

ocombe commented Oct 16, 2014

Yes, it's also taken into account for the clear method.
I could do a PR if you want.

@tofumatt
Copy link
Member

That would be great! I’m travelling in a few hours so I’ll be mostly offline until tomorrow, but I should be able to review it fast.

  • tofumatt

On October 16, 2014 at 10:53:02, Olivier Combe (notifications@github.com) wrote:

Yes, it's also taken into account for the clear method.
I could do a PR if you want.


Reply to this email directly or view it on GitHub.

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

Successfully merging a pull request may close this issue.

2 participants