Catch error when writting to local storage

[bertmad3400]

Sometimes writing to local storage will throw an error (fx. when the storage qouta is filled, it will throw "DOMException: The quota has been exceeded."). With the current codebase, this prevents the changes from reaching the store, essentially "blocking" the action. As this library is a layer on-top of the standard svelte store, this might come very unexpected for the developers using the lib, and potentially break their application. To fix it, I have taken the following approaches:

• Write to the store first, and then write to local storage. There's (in my mind at least) no reason why writing to local storage should come first, as that action would then block the browser from rendering the new UI based on the store changes, and if an error occurs, this would prevent the new value from reaching the store.
• Catch the error. There has been added a config option to allow the user to choose whether the error should be caught or not (defaults to true)

[webJose]

As a mere expectator of this project, which I like very much (that's why I watch it closely), I think this is a good addition. I would demote console.error to console.warning, as it is "degraded performance", not "non-functional problem". But that's just me.

[bertmad3400]

I see your point (and have actually really been in doubt about what the right move is), but I still believe that console.error is the right move here. The reason for this is that while the errors are "only" degraded performance for this library, these errors might very often be caused by using this library (by developers putting very large objects in a persistent store, being used to normal stores handling objects of any size), and they might break other parts of the application. The real solution here would of course be to implement proper checks and warnings to ensure that the persistent stores never fill up the storage quota, to the point where no other components could use it, but this could be a highly complicated challenge - both in terms of code complexity and having to ensure cross-browser compatibility. I think that a reasonable middle-ground is to log the messages as an error, to ensure that the developer spots them if the problem ever arises.

TL;DR: While these errors might only degrade perfomance of this lib, they might break other parts of the app.

[DevLeoko]

I think this is a great improvement, to keep the stores working even in case of persistence errors.

But I fail to see how these errors only "degrade performance". These errors do result in the value not being persisted, right? This feels like a clear functional issue. Or am I missing something?

[webJose]

But I fail to see how these errors only "degrade performance".

The store continues to work, reactivity continues to work, your application UI continues to work. Is just that the value is not persisted. Degraded perfromance. Also think about it like this: Your application is designed to work even if localStorage has no data.

A server-sided analogy would be if your Redis cache goes down. Your API continues to work in "degraded performance" mode because the cache is unavailable.

Hopefully this is clear now. 👍

[webJose]

Finally, there's the issue about people sending logs to servers. I believe an onStoreError callback is needed in the options that receives the exception for those people that send their errors to central locations.

[DevLeoko]

But I fail to see how these errors only "degrade performance".

The store continues to work, reactivity continues to work, your application UI continues to work. Is just that the value is not persisted. Degraded perfromance. Also think about it like this: Your application is designed to work even if localStorage has no data.

A server-sided analogy would be if your Redis cache goes down. Your API continues to work in "degraded performance" mode because the cache is unavailable.

Hopefully this is clear now. 👍

You have to consider that this is a very basic library and might be applied in a wide range of use cases and while I agree that in most cases it would probably just be considered degraded performance (e.g. when your theme settings (light/dark) are not persisted) but there are for sure several use cases where this poses a critical problem.

For example, in my case, I store data about a current game session in the store, and if this information is not persisted the user loses their progress and can't rejoin the current round when refreshing the tab.

[webJose]

I understand your particular case, @DevLeoko. The thing is: It is your use case. I would say that if data is this critical, it only goes to local storage for quick access but should be persisted in the back-end. Local storage is not meant to be used as a source of truth like this.

Anyway, those are my thoughts. I still have some more, but since I have no saying in the matter, I'll just be content to have left my feedback. 😄 Have a good day, guys.

[bertmad3400]

You have to consider that this is a very basic library and might be applied in a wide range of use cases and while I agree that in most cases it would probably just be considered degraded performance (e.g. when your theme settings (light/dark) are not persisted) but there are for sure several use cases where this poses a critical problem.

I would have to disagree here. The library is a very basic library, because the underlying mechanism is rather basic, and only semi-persistent (between it being error-prone and the user clearing the cache, a lot can prevent the data from being persistet). I do a 100% understand the appeal of this for something like what you're a building, but I would agree with @webJose that if the data is critical, and you intend for the project to be more than a hobby project (since hobby projects usually comes with no guarentees), you have to find a better persistence mechanism. This is not due to this library, but due to the nature of the underlying protocol (over which we obviously have no control). I still believe that it should be a console error though, for the reasons stated below, and because getting an error from this is going to be really rare, and only in cases were you are really doing something you shouldn't be doing

[joshnuss]

Thanks everyone for the feedback and comments. It is helpful!

One concern I have is with swallowing errors.

Ideally, error handling comes with a way to do something about the error.

Also, we've not had any reports of parsing failures, so I want to make sure this is really solving a problem that happens.

Has anyone encountered these types of errors?

[bertmad3400]

When I wrote this PR it was due to a very real problem in my own project that is currently limiting functionality. I would imagine the reason that you have never encountered this problem is that you have never filled up the storage quota. Besides this, I also believe that it is best practice for a library to be very aware of potential errors in event handlers as there's next to nothing the developer using the library can do about those.

Regarding doing something about the error, this PR both includes an option to not swallow the error, and - as per @webJose recommendation - also an option to specify a function to call when encountering an error.

[joshnuss]

Cool, that makes sense. I think this PR definitely needed then.

What do you think about having one optioncatchError, which accepts either a boolean or a function.

[webJose]

One concern I have is with swallowing errors. Ideally, error handling comes with a way to do something about the error.

Yes, I agree. I would add the onStoreError property to the options as an optional way to providing a callback that receives the error. Its default value would be the function that simply logs to the console:

function defaultOnStoreError<TError>(error: TError extends Error) {
    console.warn('An error occurred while trying to persist data to storage:\n%o', error);
}

Then on the catch block the code would simply call options.onStoreError or the default function.

[bertmad3400]

So based on @webJose very nice suggestion, I think I have reached a nice middleground. I have removed the catchError option, and then just have the default value of onStoreError being a logging of the error. This gives the developer using the library complete control, allowing them to re-throw the error, or to just log it as a warning instead of error. Unfortunately it is not possible to strongly type it like @webJose have done in his recent suggestion as the object returned from the catch block - for some reason - isn't an error object, and the normalization example on the MDN pages doesn't work, so I think this is the best way to do it.

[bertmad3400]

Yes, will have a look at adding tests

Edit: On a second note, I'm actually not sure how that would work. The errors I have encountered (and the only errors I can really think of) is highly browser-environment specific, and as I'm not to terribly experienced in writing tests, and the accompanying environments, I'm really not sure how I would go triggering an error

[webJose]

Hello, guys. I've been away for a bit, but I did see that you might need help with some unit testing. I checked the project out and is not really unit-testing friendly, so I made PR #219 as a POC to see if you would like to go for this kind of refactoring. It basically creates a factory of the persisted() function. Said factory currently allows the caller to pass an implementation of the isBrowser() and getStorage() functions used inside persisted(). With these, unit tests can be easily written to simulate server-side/browser code or to simulate behaviors with stores, such as thowing an error when trying to store (the current need here).

Let me know. Thanks!

[bertmad3400]

Just wrote the necessary tests... Took forever as customizing the jsdom environment in vitest on a per-file basis is incredibly poorly documented, but it should work now

[bertmad3400]

How does this look to you @joshnuss ?

[bertmad3400]

@joshnuss will it be possible to see this merged in the near future? This very error is currently having major impacts on some of our user-facing services, and if not fixed soon I would have to fork and maintain a different version of this lib (a case I would imagine we would all like to avoid given your excellent work on this)

[joshnuss]

@bertmad3400 the PR looks good. Sorry about the delay.

One thing I wasn't sure about: I think my earlier suggestion to rename onStoreError -> onError might have been off. Since the error handler is only covering write errors. I'm wondering if read errors should be handled as well?

For example, maybe we should have 2 error handlers onWriteError and onReadError.

Thoughts?

[bertmad3400]

Good point! I'm not sure an onReadError would make sense (given the only functionality I could imagine would go wrong there would be in the seralization function which the user has full control over), but will certainly be renaming the current option to onWriteError as onError is clearly too broad. Just rebased on your new changes, and will have a look at finalizing these changes tomorrow

[github-actions[bot]]

:tada: This PR is included in version 0.8.1 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: