-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Provide more of a warning if the the parsing/serializing is failing. #73
Comments
Nice, I like the improvement we can make here. About the Otherwise, what do you like best as a solution from the three proposed? |
I tend to be in favour of throwing errors, but that's perhaps my toxic trait. (It's been biting me in the ass the last couple of months). But yeah, I say default with throwing errors, and then allow users to provide their own error handling functionality. eg something like this:
I don't know. Something like You could do a 'global configuration' type solution, where rather than having to set the configuration in each hook call, you can apply a set of options that is used in all the hook calls. This is what I've done in my library react-github-permalink - for you can configure the client components via a context provider, and for RSCs you can configure by a mutable singleton. |
Ok, I like the way you are doing it — using However, isn't it true that the reason you posted the issue is because — you started using the library and there was an error you weren't seeing and you didn't understand that there was an error at all? If that's the case, would you have wired the I'm asking the questions in order to understand — are we solving the problem you had with the I also like the idea to show a warning in development but for that I will need some time to research how others do it and is it possible to do in a reliable manner. |
So the idea is, you would have some default behaviour for errors during parsing (either throwing a runtime error, or a console.error). So in my scenario I would have become aware of the issue, because I'd see the runtime error or the console.error.
Yeah so basically currently if there's a parsing error then it errors silently and reverts to using the default value. use-local-storage-state/src/useLocalStorageState.ts Lines 97 to 101 in 8d4e9c5
|
I just created a quick PR #75 - this basically adds default behaviour that will do a |
I'll split the topic in two so I make it more structured and easy to follow. Throwing and logging errors in productionTo be honest, I'm hesitant to include any throwing of errors and
My experience and intuition lean toward open-source packages that don't get in the way. In projects, we rely on so many npm packages that we should think in a way of "what if all packages did that". Good example is the ecosystem around the https://github.com/debug-js/debug package. This a possible way we can approach this but you can see it's opt-in and not turned on by default.
|
Having thought about it - I think your approach of 'if it encounters a parsing error, then use the default value' makes sense - it's just like no localstorage value existed at all. Re: breaking changes - I think it would still be nice to provide some kind of console warning. Out of an abundance of caution, you could include the warn behaviour as default in the next major version bump, and allow users to opt out of it by provide their own error handling function. You could add the error handling functionality now, just without the console.warning as default behaviour. |
Yeah, after I've thought about it as well, these all seem like good points. I'll schedule some time to work on this. Thanks for everything! |
Just spent some time debugging and issue where I'm writing a test like:
And wondering why it didn't seem to be respecting the existing localStorage values.
Turns out because I hadn't provided a
serializer
property in the options object, the defaultparseJSON
serialization is throws, and then the default value is used.It would be to be aware that this is happening and either:
The text was updated successfully, but these errors were encountered: