Wisembly / basil.js

The missing Javascript smart persistent layer
https://wisembly.github.io/basil.js
Other
1.98k stars 62 forks source link

Issue with namespace "reset" logic #69

Open Pranjali14 opened 4 years ago

Pranjali14 commented 4 years ago

Steps to reproduce -

I went through the code and observed that the way the "reset" function is implemented

reset: function (namespace) {
                for (var i = 0, key; i < window[this.engine].length; i++) {
                    key = window[this.engine].key(i);
                    if (!namespace || key.indexOf(namespace) === 0) {
                        this.remove(key);
                        i--;
                    }
                }
    }

In this case key.indexOf(namespace) === 0 for namespace and namespacePersistent will be true and hence will reset both the namespaces.

Solution : We can use a combination of namespace and delimiter for finding the index. Considering the default delimiter as ' . ' a namespace delimiter combination will not return 0

let key = 'namespacePersistent.key1' ;
key.indexOf("namespace.") === 0 // false since index returned will be -1.

We can pass the namespace and delimiter combination from the callee.

reset: function (options) {
                options = Basil.utils.extend({}, this.options, options);
                Basil.utils.tryEach(_toStoragesArray(options.storages), function (storage) {
                                           let namespaceStr = options.namespace+(options.keyDelimiter || '.'); 
                    _storages[storage].reset(namespaceStr);
                }, null, this);
            },

If this solution is looks fine I can raise a PR for the same.

guillaumepotier commented 4 years ago

Hi @Pranjali14

Thanks for this issue, very clear and even with envisaged solution :)

Indeed, we could easily replace key.indexOf(namespace) === 0 by key.indexOf(namespace + (options.keyDelimiter || '.' )) === 0 to ensure being more strict and make your case working.

Would you made making a PR with that change, and if possible, even with a test in the test suite (with your above example?)

Thanks

Pranjali14 commented 4 years ago

Sure @guillaumepotier will make the changes and update the test suite as well.

Thanks

Pranjali14 commented 4 years ago

Hi @guillaumepotier,

I have raised a PR for this issue.

Thanks, Pranjali