Handle web storage DOMException

[ckopanos]

Hi, Thanks for the handy library.

I came over an issue trying to use localStorage when the values stored exceeded the browser quota. Since there was no exception handling in the set item method, this made the application break with a DOMException, and consequent requests of the app to an http api would also misbehave.

The current pull request addresses handling exceptions in the set method of buildWebStorage when using sessionStorage or localStorage APIs.

In case the DOMException is a QuotaExceededError the storage is cleared to allow the application to store new data.

A possible better approach would be to just scan the keys and only delete the ones that have expired until storage quota is less than the browser quota, but considering the synchronous behavior of localStorage and sessionStorage it could slow down the app or even freeze the browser (depending on the number of keys stored), not to mention the need to address fetching the browser quota using the StoreManager API to determine when we are below quota and if the value or consequent values to store would not raise new issues.

Therefore just clearing the storage should be a good compromise for most applications.

Let me know if you have any concerns over this. Writing typescript for the first time so you may encounter a couple of things possibly.

[arthurfiorette]

Hey! Thanks for opening this issue with a fix together!

I came over an issue trying to use localStorage when the values stored exceeded the browser quota. Since there was no exception handling in the set item method, this made the application break with a DOMException, and consequent requests of the app to an http api would also misbehave.

I had this problem a long time ago, and I ended up forgetting about it. Thank you again!

In case the DOMException is a QuotaExceededError the storage is cleared to allow the application to store new data.

Well, I went up to search about it and, appearently, there are many possible error messages for many browsers. This blog post suggests to, basically, just catch everything.

A possible better approach would be to just scan the keys and only delete the ones that have expired until storage quota is less than the browser quota, but considering the synchronous behavior of localStorage and sessionStorage it could slow down the app or even freeze the browser (depending on the number of keys stored), not to mention the need to address fetching the browser quota using the StoreManager API to determine when we are below quota and if the value or consequent values to store would not raise new issues.

Therefore just clearing the storage should be a good compromise for most applications.

This might be true, but don't you think that it requires a really LOT of keys to freeze a browser for a second? Well, it'll require a benchmark to prove anything. But, what about adding an option (clearOnError) to toogle this behaviour.

Instead of just removing the expired ones (which includes verifying if the cache can be stale), it can be broken into multiple steps:

1. First remove all expired keys that cannot be stale.
2. Tries to save the new object.
3. If got error, remove the last created cache.
4. Tries again to save the new object.
5. Repeat step 3..4 until the object can be saved or the cache got empty.
6. If the cache is empty and the object couldn't be saved, likely because it is higher than the quota, just ignores it.

In case the clearOnError option is true, the order should be 1,2,storage.clear().

[ckopanos]

Hey again, appreciate the comments..

This might be true, but don't you think that it requires a really LOT of keys to freeze a browser for a second? Well, it'll require a benchmark to prove anything. But, what about adding an option (clearOnError) to toogle this behaviour.

clearOnError makes sense true. Maybe some people will want to handle this on their own indeed. They may indeed want to e.g. not store more results and that could be a case under certain use cases.

Regarding browser freezes you are right, maybe I was a bit biased based on my use case on that, when I did experience browser freezes with firefox but still like you said performance checking would suggest the way to go.

Now coming to your final point about introducing a set of steps in order to avoid totally clearing the cache.

I am not that familiar with the library internal (so I may be wrong and the library already covers the following),
but wouldn't the steps you propose introduce a prerequisite in the sense that since on set of a specific key value pair we do not know the rest of the keys already stored and since (as far as I know) the storage apis do not introduce some kind of list or whatever other method to get the stored keys, a new (internal or configurable) key for storing the keys and ttls would be needed.

This way having such a key value pair would (even if there was a list method in the storage API), allow the system to more quickly filter against the value stored in that key, to find the last or the expiring keys and clear them.

Of course this means that on application load (typically visiting or refreshing the page) the app would need to load the value of that key in memory from storage, then on each key set method, update the in memory structure of the keys stored and persist that on the storage so that steps 1 - x can follow when needed in case the clearOnError preference is not true.

I'll see to work on the clearOnError preference.
Just a note, I do have spare time mainly on weekends.. Would you consider the pull request with the clearOnError preference implemented and then the implementation of clearing in case clearOnError is false can follow at another stage? Handling any error would guard against applications not working anyway, the system though with this lets say partial implementation would not store more keys...

[arthurfiorette]

I am not that familiar with the library internal (so I may be wrong and the library already covers the following),
but wouldn't the steps you propose introduce a prerequisite in the sense that since on set of a specific key value pair we do not know the rest of the keys already stored and since (as far as I know) the storage apis do not introduce some kind of list or whatever other method to get the stored keys, a new (internal or configurable) key for storing the keys and ttls would be needed.

You can use Object.keys(storage) or a loop for (const key in storage) {}, and then filter all keys that starts with the provided prefix. This may help.

[arthurfiorette]

Just a note, I do have spare time mainly on weekends.. Would you consider the pull request with the clearOnError preference implemented and then the implementation of clearing in case clearOnError is false can follow at another stage?

Np! I'm working on this PR too

[ckopanos]

I am not that familiar with the library internal (so I may be wrong and the library already covers the following),
but wouldn't the steps you propose introduce a prerequisite in the sense that since on set of a specific key value pair we do not know the rest of the keys already stored and since (as far as I know) the storage apis do not introduce some kind of list or whatever other method to get the stored keys, a new (internal or configurable) key for storing the keys and ttls would be needed.

You can use Object.keys(storage) or a loop for (const key in storage) {}, and then filter all keys that starts with the provided prefix. This may help.

But that will return all the keys stored in object or sessionStorage wont it? among which possible other keys not "owned" by the cache interceptor, therefore it will also require scanning the content of the keys to make sure its "owned" or not... Even with checking the content what about clashes with other values with similar in nature structure?

[arthurfiorette]

Yep, and to avoid this key conflict problem, there's the prefix option on buildWebStorage. This way it can "blindly" search for all keys that starts with the specified prefix and use it normally.

[ckopanos]

Yes just saw the code... you have already fixed most of the things you mentioned.. Great!

[arthurfiorette]

Also, i thought for a while about the clearOnError option and decided to remove it.

The default (and desired) behaviour is to handle quota errors. And makes no sense in the first error to just clear all other "healthy" keys because of the quota.

So, if someone needs this "unique" behavior, they can just override the buildWebStorage with their needs.

[ckopanos]

Also, i thought for a while about the clearOnError option and decided to remove it.

The default (and desired) behaviour is to handle quota errors. And makes no sense in the first error to just clear all other "healthy" keys because of the quota.

So, if someone needs this "unique" behavior, they can just override the buildWebStorage with their needs.

This is true.. Great

[ckopanos]

Sorry, closed the pr accidentally instead of comment.. Saw your comment makes sense

[arthurfiorette]

Sorry, closed the pr accidentally instead of comment..

This gets me every time too 🤣🤣

[codecov]

Codecov Report

Merging #148 (b42e111) into main (97c38ba) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #148   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        17    +1     
  Lines          428       493   +65     
  Branches       143       145    +2     
=========================================
+ Hits           428       493   +65     

Impacted Files	Coverage Δ
src/storage/build.ts	100.00% <100.00%> (ø)
src/storage/web-api.ts	100.00% <100.00%> (ø)
test/storage/quota.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c38ba...b42e111. Read the comment docs.

[arthurfiorette]

Hey @ckopanos, everything ok or there's anything more to do?

[ckopanos]

The only thing I can think of (unless your typesscript object declarations will guard against it, sorry not familiar with TS) is the case when prefix === ''.
If that's the case and the user has not proactively set a prefix then

const allValues: [string, StorageValue][] = Object.entries(
          storage as Record<string, string>
        )
          .filter(([key]) => key.startsWith(prefix) && storage.getItem(key))
          .map(([key, val]) => [key, JSON.parse(val) as StorageValue]);

        // Remove all expired values
        for (const [prefixedKey, value] of allValues) {
          if (value.state === 'cached' && isExpired(value) && !canStale(value)) {
            storage.removeItem(prefixedKey);
          }
        }

allValues will return all keys ("owned" and not) in local or sessionStorage which then on will either cause issues with

JSON.parse(val) as StorageValue => won't match type

Or
properties in value will be undefined rendering methods such canStale throw value.data is undefined

[ckopanos]

The order of if checking will not cause issues with canStale though

[ckopanos]

Just for clarification I am not referring to "not owned" as keys stored using a e.g. second buildWebStorage method to be used in other axios instances, but other possible plain keys in localStorage been used in other parts of the app not related to axios or the interceptor

[arthurfiorette]

That's why prefix exists. With your examples, I think it's better set an non-empty default prefix like: __ACI.

But will it be a breaking change?

[ckopanos]

That's why prefix exists. With your examples, I think it's better set an non-empty default prefix like: __ACI.

But will it be a breaking change?

Honestly yes.. it will be breaking true.. suddenly everyone will have their stored keys rendered unusable if a default prefix is introduced yes. Since you have already mentioned the need for a prefix in your documentation then I guess in case of errors other devs will figure out..

It depends on each case.. Would I value more existing keys been rendered unusable once a default prefix was introduced or my app breaking for because I had other keys stored not related to axios? Personally I would prefer my app to continue to work even if that meant that I would loose the speedup gain cause some data would need to be fetched again from the remote.

But in the end its up to you.. You cal always just keep it as it is and highlight the possible case with your release notes.

[arthurfiorette]

It depends on each case.. Would I value more existing keys been rendered unusable once a default prefix was introduced or my app breaking for because I had other keys stored not related to axios? Personally I would prefer my app to continue to work even if that meant that I would loose the speedup gain cause some data would need to be fetched again from the remote.

You have a very strong case, i'm going to change and add a note on the next release.