useLocalStorageHook set defaultValue to undefined but get empty string

[Eriice]

What package has an issue

@mantine/hooks

Describe the bug

Hello, why choose Nullish coalescing operator to make defaultValue which is undefined or null to empty string in useLocalStorageHook?

It's a little weird if I expect an undefined/null defaultValue but get an empty string.

---

Also, is there a typo here in use-local-storage?

If set to true, value will be update is useEffect after mount

If set to true, value will be update in useEffect after mount

What version of @mantine/hooks page do you have in package.json?

latest

If possible, please include a link to a codesandbox with the reproduced problem

https://codesandbox.io/s/kind-star-v25pry?file=/src/App.tsx

Do you know how to fix the issue

Yes

Are you willing to participate in fixing this issue and create a pull request with the fix

Yes

Possible fix

const readStorageValue = useCallback(
      (skipStorage?: boolean): T => {
        if (typeof window === 'undefined' || !(type in window) || skipStorage) {
          return defaultValue as T;
          // return (defaultValue ?? '') as T;
        }

        const storageValue = window[type].getItem(key);

        return storageValue !== null ? deserialize(storageValue) : (defaultValue as T);
        // return storageValue !== null ? deserialize(storageValue) : ((defaultValue ?? '') as T);
      },
      [key, defaultValue]
    );

[Narcha]

@rtivital what are your thoughts on this?
The suggested fix would make this hook much easier to work with.
Especially for values where "unset" is a valid option.

I can create a PR if needed.

[rtivital]

I'm fine with this, feel free to create a PR

[Narcha]

I have a question about how to implement this.
If the stored value is of type T, the default Value should be of type T | undefined, because it is optional.
Now, should the return type of the hook be of type T too?
If the value is not found and the default is undefined, this would violate that type constraint.

I don't like setting the return type to T | undefined either, because if the user specified a default value of type T,
the value can never be undefined and this type would just create extra work for the user.

An alternative I considered was to add a second generic type parameter:
createStorage<T> would become createStorage<T, D> with D being the type of the default value.
Thus, the return type can be T | D, which would just be T in case the default is of type T, but this would communicate the possibility of undefined as a return value in case the default value is set to undefined.

What do you think? @rtivital

[rtivital]

User should specify value type on their side:

const [colorScheme, setColorScheme] = useLocalStorage<ColorScheme | undefined>({
    key: 'color-scheme',
    defaultValue: undefined,
  });

[Narcha]

I see. This does leave the possibility of misuse by the user,
but if that's by design I'll just implement the simple option of returning T.

[rtivital]

This does leave the possibility of misuse by the user

Why? As I see it, the user defines the type and gets exactly what is defined.

[Narcha]

If the user forgets to add a default value and calls useLocalStorage<ColorScheme>,
I think this would return undefined, which is not of type ColorScheme.

[rtivital]

That is an error on user side, on library level we should not assume that the user will make errors in their code. It will also make harder for the users who did provide default value. For example, in this case useLocalStorage({ key: '1', defaultValue: '2' }), value will never be undefined, but the type will include it, which is not accurate.

[Narcha]

In this example, if the return type would be of type T | D like in my suggested approach,
D would be the type "2", and the return type would be string | "2", or just string.
In the same example, if the user does not specify a default value, D will be undefined,
and the return type T | D would be string | undefined.

I think this accurately models the situation, and I don't think the type would include unneeded undefineds

[Narcha]

I created #2872.