Skip to content
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

State Setters from useMMKV hooks shoud support retrieval of previousState #573

Open
fahad86 opened this issue Aug 21, 2023 · 9 comments
Open

Comments

@fahad86
Copy link
Contributor

fahad86 commented Aug 21, 2023

This is to help with having a clean code with minimal dependancies (and probably fewer re-renders)

For e.g.

const [user, setUser] = useMMKVObject("user");

let newName = "BLAHBLAH";

setUser((prevState) => {
  return {
    ...prevState,
    name: newName
  };
});
@mrousavy
Copy link
Owner

Good point, but this will be an extra read. What if the user doesn't need the previou state? then it will be an unnecessary DB Read

@fahad86
Copy link
Contributor Author

fahad86 commented Sep 19, 2023

Is there a way to maybe overload the method (https://www.geeksforgeeks.org/function-overloading-in-javascript/) in such a way that if an arg (prevState) is provided then we do the extra read and perform extra processing and if not then we just maintain the original behaviour?

@mrousavy
Copy link
Owner

mrousavy commented Sep 20, 2023

Wait hold on - isn't that already possible?

You can do

const [username, setUsername] = useMMKVString("username")

setUser((prevState) => {
  if (prevState === 'marc')
    return 'fahad86'
  else
    return 'marc'
});

Here's the code for that

const newValue = typeof v === 'function' ? v(valueRef.current) : v;

@mrousavy
Copy link
Owner

Aha, it works for everything but not useMMKVObject yet. got it.

export function useMMKVObject<T>(
key: string,
instance?: MMKV
): [value: T | undefined, setValue: (value: T | undefined) => void] {
const [string, setString] = useMMKVString(key, instance);
const value = useMemo(() => {
if (string == null) return undefined;
return JSON.parse(string) as T;
}, [string]);
const setValue = useCallback(
(v: T | undefined) => {
if (v == null) {
// Clear the Value
setString(undefined);
} else {
// Store the Object as a serialized Value
setString(JSON.stringify(v));
}
},
[setString]
);
return [value, setValue];
}

@fahad86
Copy link
Contributor Author

fahad86 commented Sep 21, 2023

Aha, it works for everything but not useMMKVObject yet. got it.

export function useMMKVObject<T>(
key: string,
instance?: MMKV
): [value: T | undefined, setValue: (value: T | undefined) => void] {
const [string, setString] = useMMKVString(key, instance);
const value = useMemo(() => {
if (string == null) return undefined;
return JSON.parse(string) as T;
}, [string]);
const setValue = useCallback(
(v: T | undefined) => {
if (v == null) {
// Clear the Value
setString(undefined);
} else {
// Store the Object as a serialized Value
setString(JSON.stringify(v));
}
},
[setString]
);
return [value, setValue];
}

I assumed it didn't work for the other setters as well😅.. so what's the plan now?

@bviebahn
Copy link

bviebahn commented Dec 5, 2023

Is there any reason this is not implemented yet? I currently just use my own hook which is pretty straightforward:

function useMMKVObject<T>(key: string) {
    const [json, setJson] = useMMKVString(key);

    const value = React.useMemo<T | undefined>(
        () => (json ? JSON.parse(json) : undefined),
        [json],
    );

    const setValue = React.useCallback(
        (v: (T | undefined) | ((prev: T | undefined) => T | undefined)) => {
            if (v instanceof Function) {
                setJson((currentJson) => {
                    const newValue = v(
                        currentJson ? JSON.parse(currentJson) : undefined,
                    );
                    return JSON.stringify(newValue);
                });
            } else {
                setJson(v ? JSON.stringify(v) : undefined);
            }
        },
        [setJson],
    );

    return [value, setValue];
}

@fahad86
Copy link
Contributor Author

fahad86 commented Jan 18, 2024

@mrousavy the above solution by @bviebahn looks good right? There is also a related PR: https://github.com/mrousavy/react-native-mmkv/pull/254/files

@mrousavy
Copy link
Owner

Hey - that PR is outdated, can someone create a new PR with those changes? Then I think we can merge it, however: this is unsafe as there might've been outside modificiations to that data type (e.g. from a notification extension, data loss, or whatever) and the resulting type might not satisfy the type T anymore - so use it at your discretion.

@fahad86
Copy link
Contributor Author

fahad86 commented Jan 18, 2024

understood. PR created: #623 please test to ensure that you don't see any issues in your side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants