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

[dwjohnston]

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]

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]

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]

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]

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.

use-local-storage-state/src/useLocalStorageState.ts

Lines 97 to 101 in 8d4e9c5

	try {
	parsed = string === null ? defaultValue : (parse(string) as T)
	} catch {
	parsed = defaultValue
	}

[dwjohnston]

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]

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]

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.

[dwjohnston]

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.

[astoilkov]

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!