Bug in deletePrefix().

[boxymoron]

The implementation of deletePrefix() is akin to removing an item from an array while iterating (i.e. doesn't work).
Possible Fix?:

public deletePrefix(prefix: string): void {
  Object.keys(this.ls).filter(k => k.startsWith(prefix) && k !== this.recentKey).forEach(k => {
    this.deleteUsage(k);
    this.ls.removeItem(k)
  });
}

[williamstein]

Your suggestion looks good to me, except with

try {
      this.deleteUsage(key);
      this.ls.removeItem(key);
    } catch (e) {
      console.warn(`localStorage: delete error -- ${e}`);
    }

to maintain the same semantics.

[williamstein]

@boxymoron do you want to make a PR?

[haraldschilly]

I've created a PR, so far just modified the test. I think I understand your point, but my test doesn't trigger a problem. Maybe I'm confused 🤔

#4

[haraldschilly]

@boxymoron ok. can you please check my #4 PR?

[boxymoron]

@haraldschilly
Try running this in a browser (I used the latest Firefox):

console.log('Begin LocalStorageLRU.deletePrefix() test:')
      console.log('clearing localStorage...')
      window.localStorage.clear();
      console.log('window.localStorage.length: ', window.localStorage.length);
      lsLRU = new LocalStorageLRU({
        maxSize: 512,
        recentKey: 'myKey',
        fallback: false,
      });
      
      lsLRU.set('foo', 'bar');
      for(let i=0; i<100; i++){
        lsLRU.set('test-'+i, i)
      }
      console.log('lsLRU.size(): ', lsLRU.size(), ', should be 101');
      
      console.log('calling LocalStorageLRU.deletePrefix()')
      lsLRU.deletePrefix('test-');
      console.log('lsLRU.size(): ', lsLRU.size(), ', should be 1')

The output is:

Begin LocalStorageLRU.deletePrefix() test:
clearing localStorage...
window.localStorage.length:  0
lsLRU.size():  101 , should be 101
calling LocalStorageLRU.deletePrefix()
lsLRU.size():  50 , should be 1

Also, I found another issue, LocalStorageLRU.clear() calls localStorage.clear(). While this is documented with a comment as such, I think it is problematic, as users of the library probably expect it to only clear entries "managed" by LocalStorageLRU, while it clears everything in localStorage (including key/values that were not "managed" by LocalStorageLRU). Maybe this merits another PR?

[haraldschilly]

@boxymoron so, thank you for explaining this. Version 2.5.0 is released. Can you please check if this works for you?

Regarding clear(): yes, this lib is kind of a complete take over. Maybe this should be explained in the readme a little bit more? There is no tracking of which keys this lib has under its control. It's also using existing entries that already existed.

The only thought I have is: there could be a default prefix (by default, there is none, to match the current behavior) and all get/set/delete ops add that common prefix to all keys automatically. That way, this lib would have its own "namespace".

[boxymoron]

@haraldschilly Aren't all the keys tracked in the recentKey entry itself? You should be able to do something like:

localStorage.getItem(recentKey).split(delimiter).forEach(k => localStorage.removeItem(k));

[boxymoron]

@boxymoron so, thank you for explaining this. Version 2.5.0 is released. Can you please check if this works for you?

Regarding clear(): yes, this lib is kind of a complete take over. Maybe this should be explained in the readme a little bit more? There is no tracking of which keys this lib has under its control. It's also using existing entries that already existed.

The only thought I have is: there could be a default prefix (by default, there is none, to match the current behavior) and all get/set/delete ops add that common prefix to all keys automatically. That way, this lib would have its own "namespace".

@haraldschilly Yes, it's working correctly now. Thank you for updating.

[haraldschilly]

@haraldschilly Aren't all the keys tracked in the recentKey entry itself?

well, no, only the recent ones. It's used by the trim() method, and well, the background of all that is to implement an LRU cache – hence the name. The root problem we faced was, when the local storage gets full, exceptions are thrown, you can't save anything new anymore. This wrapper records recent usage (keys) and updates the list of recent keys. If that update fails, it issues a trim operation (which heuristically removes some entries, but not the recent ones). Hence, it is always possible to save/update keys, while it eventually forgets some of the old stuff. The due to all that, performance penalty is very small, and the storage overhead is constant (and very minimal as well).