Provide more of a warning if the the parsing/serializing is failing.

dwjohnston commented 2 weeks ago

Just spent some time debugging and issue where I'm writing a test like:

function TheComponent() {
    const [value, setValue] = useLocalStorageState("storage-key"); 

    return <div>{value}</div>
}

//test 

localStorage.set("storage-key", "2024-05-01T12:00:00Z"); 
render(<TheComponent/>)

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 default parseJSON serialization is throws, and then the default value is used.

It would be to be aware that this is happening and either:

(easy) - console.warn to tell the developer that serialization has failed
(breaking change) - Throw an error if serialization fails. Provide an 'opt out of errors' option if the user wants to supress this option.
(non-breaking middleground) - provide an opt in - 'throw if serialization fails' option.

astoilkov commented 2 weeks ago

Nice, I like the improvement we can make here.

About the console.warn() option: Do you know how I can easily (without making production and development build) show a warning only in development?

Otherwise, what do you like best as a solution from the three proposed?

dwjohnston commented 1 week ago

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:

    const [value, setValue] = useLocalStorageState<string>("storage-key", {
         onError: (err: unknown, newValue: string, oldValue: string> : string  => {
              sentry.report("some kind of third party logging here"); 

              return "the value to return in this scenario"; 
              // Or continue to throw an error

         }
    });

About the console.warn() option

I don't know. Something like process.env.NODE_ENV isn't going to work for a browser bundle. I think this kind of thing is always going to depend on the context that the code is being used only, some users might want to be providing their own logger, etc.

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.

astoilkov commented 1 week ago

Ok, I like the way you are doing it — using onError.

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 onError and seen the error?

I'm asking the questions in order to understand — are we solving the problem you had with the onError or another one.

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.

dwjohnston commented 1 week ago

If that's the case, would you have wired the onError and seen the error?

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. To opt out of the default behaviour, the user provides their own onError.

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?

Yeah so basically currently if there's a parsing error then it errors silently and reverts to using the default value.

https://github.com/astoilkov/use-local-storage-state/blob/8d4e9c5f3a3d61d0498f2e6576627901c9a76f35/src/useLocalStorageState.ts#L97-L101

dwjohnston commented 1 week ago

I just created a quick PR #75 - this basically adds default behaviour that will do a console.error, as well as customise what the fallback value should be if it errors.

dwjohnston commented 1 week ago

PR #76 goes one step further - if the onError function throws an error, then the hook will throw this as a runtime error.

You could adopt #76 - but opt for console.error as the default error strategy, (ie. let users choose if they want to warn and set a default value, or just blow things up).

astoilkov commented 1 week ago

I'll split the topic in two so I make it more structured and easy to follow.

Throwing and logging errors in production

To be honest, I'm hesitant to include any throwing of errors and console calls that's not restricted to development. This is because:

Throwing an error and breaking a company website because of some small npm package is not something I want happen
Logging is less of a problem, but imagine many of the small npm packages you've installed started throwing warnings. Here the error should be rare and I feel like we can be fine but I want to think this through and find examples of popular open-source repos that log stuff.

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.

`onError`

I like the idea behind onError. However, during development you can just enable "Pause on caught exceptions" in DevTools and catch any errors coming from this hook. I feel like I want onError to be useful in production and enable a use case that would otherwise be impossible using the current API.

I would love you or someone else to show a use case where onError will deliver a real production feature that will be useful. This way this feature will be a lot easier to justify.

Conclusion

I'm very grateful for your work on the PRs and I feel like I want to merge them but I'm currently still processing the information and trying to figure out things and don't have any final decision what and if I should do something about this.

This library is quite popular and I'm more careful than usual. I hope you can understand me.

If anybody else sees this discussion and is interested, please feel free to tune in and share your opinion so we can understand this better.

astoilkov / use-local-storage-state