ammarahm-ed / react-native-mmkv-storage

An ultra fast (0.0002s read/write), small & encrypted mobile key-value storage framework for React Native written in C++ using JSI
https://rnmmkv.now.sh
MIT License
1.58k stars 109 forks source link

[Question] useMMKVStorage typing not mimicking useState #253

Closed douugdev closed 2 years ago

douugdev commented 2 years ago

Hello! Firstly I wanna thanks @ammarahm-ed for this awesome library and work since I think this is my first time posting here, you're a life-saver. By the way, I don't know how to reference files so I'll post codeblocks along with paths.

From my understanding, the useMMKVStorage hook is made to mimic the behaviour of useState but with persistent storage, if that's the case then there is some weird typing behaviour which it's an easy fix, but the reason I'm not opening a PR is because of the first part of this sentence: it's my understanding, so I don't really know if this is working as intended or not:

On /src/hooks/useMMKV.ts line 60, the typing for useMMKVStorage is the following:

const useMMKVStorage = <T>(
  key: string,
  storage: MMKVInstance,
  defaultValue?: T | null | undefined // <-- Here
): [
  value: T | null | undefined, // <-- And here
  setValue: (value: T | ((prevValue: T | null | undefined /* <-- Aand here */) => T)) => void
] => {...}

See, with the type definitions of react, useState uses function overloading to type accordingly if you have passed or not the defaultValue:

const [value, setValue] = useState<string>() // Type of value: string | undefined
const [value, setValue] = useState('default') // Type of value: string

because if you have a default value, the value returned must not be null or undefined, that's where useMMKVStorage differs:

const [value, setValue] = useMMKVStorage<string>('key', storage) // Type of value: string | null | undefined, currently fine
const [value, setValue] = useMMKVStorage<string>('key', storage, 'default') // Type of value: string | null | undefined, how can value ever be null or undefined if it has a default value?

And also is it intentional to change the default value from undefined to null on /src/hooks/useMMKV.ts line 157?

   defaultValue = defaultValue === undefined ? null : defaultValue;
   return [valueType === 'boolean' ? value : value || defaultValue, setNewValue];

Well those are all my questions/points, thanks for reading :)

ammarahm-ed commented 2 years ago

Hey @douugbr thanks for opening an issue. Yes useMMKVStorage mimics the behavior of useState hence types should work in the same way. A PR would be great.

When you make a PR, don't forget to update types for create function too.

The conversion of defaultValue from undefined to null is because otherwise the value returned from the hook differs from the the normal get function like getString if the user has not passed a default value to the hook. If there is a better way to do this, let me know.

douugdev commented 2 years ago

A PR is on its way!

Regarding the defaultValue being null, I asked from the POV of a design decision rather than implementation, since useState's default fallback when the user hasn't passed the default value is undefined instead of null.

ammarahm-ed commented 2 years ago

@douugbr I requested some minor changes. It looks great btw! Thanks