yarkovaleksei / vue2-storage

Wrapper over browser storage for JavaScript or Vue.js app
http://yarkovaleksei.github.io/vue2-storage/
MIT License
112 stars 11 forks source link

Asymmetry in key prefix #46

Closed halaprop closed 3 years ago

halaprop commented 4 years ago

Hi -

this.$storage.set('key', 'value')
let keys = this.$storage.keys() // --> ['app_key']
let value = this.$storage.get(keys[0])

I expect value to == 'value', but value is null. It is null because addPrefix() unconditionally adds the app prefix to the key, so get tries to get 'app_app_key'.

I think keys() should strip the prefixes, so this.$storage.keys() produces ['key'].

This might be a breaking change, so an alternative fix would be to have keys() return prefixed keys (no change) and have addPrefix(key) conditionally add a prefix, only if it isn't already part of the key.

halaprop commented 4 years ago

FYI, these are the few changes I made to the module to make it work better for me, resolving the few issues I mentioned (I don't use GitHub too much, so I was too lazy to learn about branching)...

1) Allow a replacer function...

    setOptions (config = {}) {
      const defaultOptions = {
        prefix: 'app_',
        driver: StorageDriver.LOCAL,
        ttl: 0,
        replacer: (key, value) => value  // new
      };
      const options = objectAssign(defaultOptions, config);
      this.options = Object.freeze(options);
    }

    toJSON (data, options = {}) {
      const ttl = ('ttl' in options) ? options.ttl : this.options.ttl;
      return JSON.stringify({
        value: data,
        ttl: ttl > 0 ? ttl + Date.now() : 0,
      }, options.replacer);  // new
    }

2) Have keys honor the ttl

    keys () {
      let driver = this.options.driver !== StorageDriver.MEMORY ? this.driver : this.driver.storage
      const keys = Object.keys(driver);
      return keys.filter(key => this.fromJSON(key) != null)
    }

3) Treat prefixes symmetrically.

    addPrefix (key) {
      return `${this.options.prefix || ''}${this.removePrefix(key)}`;  // added removePrefix
    }
yarkovaleksei commented 4 years ago

@halaprop , thank you for your comment. Yes, indeed, I did not take into account this nuance. Will definitely fix it soon.

halaprop commented 4 years ago

@yarkovaleksei - I updated the keys() method in my comment above. It had a typo and didn't handle both types of storage.

Also note that my idea is a potentially costly check for keys with large object values. Since the timeout (ttl) is stored with the clients saved object, the object must be JSON parsed to get the timeout. This slows the keys method.

Another design idea would be to store the timeouts elsewhere in storage under a protected key. So storage would look like this:

{
    $$$internal_meta_data_key: {
        app_key1: { ttl: 6000 },
        app_key2: { ttl: 120000 }, ...
    },
    app_key1: { here is the pure object that client stored, without a ttl }, ...
}

Now, every key access would start by checking $$$internal_meta_data_key for the key, and checking the timeout. If a timeout is discovered, clean up the keys and the stored object.