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

Issue with namespace "reset" logic #69

Open
Pranjali14 opened this issue May 19, 2020 · 3 comments
Open

Issue with namespace "reset" logic #69

Pranjali14 opened this issue May 19, 2020 · 3 comments

Comments

@Pranjali14
Copy link

Pranjali14 commented May 19, 2020

Steps to reproduce -

  • Create two namespaces for the same application namely - "namespace" and "namespacePersistent".
  • Try to reset "namespace".
  • Observe that even "namespacePersistent" gets reset.

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
Copy link
Member

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
Copy link
Author

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

Thanks

@Pranjali14
Copy link
Author

Hi @guillaumepotier,

I have raised a PR for this issue.

Thanks,
Pranjali

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

No branches or pull requests

2 participants