-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat: add reviver function to JSON.parse calls #279
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hi, I apologize for not thinking thoroughly before suggesting to send a PR earlier. The library already has transaction manager which is able to do the above. For example in your case: MMKV.transcations.register("object", "onread", ( key, value) => {
if (key === "date") {
value.date = new Date(value.date);
return value;
}
return value;
}); This works across the app in hooks and everywhere you try to retrieve the value. I apologize again because you have done a lot of work in this PR. I should have read your suggestions in the issue more thoroughly.
The fixes you made to the docs could be part of a separate PR that addresses that issue specifically. Let me know your thoughts. |
Hey! Don't worry, I'm happy to contribute in any way and making this PR made me learn more about the inner workings of this project. (Which I should've read more carefully to find the transactions class haha) The transaction manager solves my problem for a default reviver that is global throughout the instance, but it would be nice to implement your previous suggestion as well, as it resembles how // Sometimes I just want to use different reviver functions
// on each particular call, or use none to gain performance
JSON.parse(stringified, reviverFunc);
JSON.parse(stringified, reviverFunc2);
JSON.parse(stringified);
// Changing `JSON.parse` to `getMap` and `stringified` to an stored key
// works in a similar way with this PR
// This lib's current counterpart adds some boilerplate:
MMKV.transactions.register('object', 'onread', reviverFunc);
MMKV.getMap('mykey');
MMKV.transactions.register('object', 'onread', reviverFunc2);
MMKV.getMap('mykey');
MMKV.transactions.unregister('object', 'onread');
MMKV.getMap('mykey');
// As for the useMMKVStorage hook, it's confusing how using two revivers would work:
const [value, setValue] = useMMKVStorage(storage, default);
const [value2, setValue2] = useMMKVStorage(storage, default);
// Will the transactions manager register before it reads from cold storage on startup?
useEffect(() => {
MMKV.transactions.register('object', 'onread', reviverFunc);
}, [value]);
useEffect(() => {
MMKV.transactions.register('object', 'onread', reviverFunc2);
}, [value2]) Surely it will be a lot easier to rewrite now with the knowledge of the how transaction manager works. Also I'd sure like to try to implement the mutator to the I'll be closing this PR since even if you find this change relevant, I'll have to rewrite most of the code. |
Since the You can use // We register 'object'/'onread' transaction mutator only once in the app.
MMKV.transactions.register("object", "onread", ( key, value ) => {
if (key === "date") {
value.date = new Date(value.date);
return value;
} else if (key === "xyz") {
return value;
}
return value;
}); And so on for each transcation type. The performance overhead of the mutator function should be very little if the key does not match and it just returns the value back. Another way to optimize this would be to have a key registry inside transaction manager like this: // We register 'object'/'onread' transaction mutator only once in the app.
MMKV.transactions.register("object", "onread", ( key, value ) => {
if (key === "date") {
value.date = new Date(value.date);
return value;
} else if (key === "xyz") {
return value;
}
return value;
}, ['key1','key2','key3']); Then the internal function can just return the value as is if |
Since I don't usually check by key in the reviver, rather by matching the value to a regex function or converting the string to a Date object then checking if the object is a valid date, I think performance can still be an unnecessary problem for big objects and arrays, so the key registry idea is probably the best solution for when I'm certain I'll only have to revive certain stored keys. Can I work on it? |
Yes, you can work on it. Remember that registered keys will be bound to each type of transaction which is similar to how |
Hi again!
This PR Fixes #277
I reckon it's a big one so although these changes are not breaking ones and are working as intended, I need more opinions on both the design and implementation parts, I'm happy to discuss other ideas and change the code accordingly!
I implemented both of @ammarahm-ed's ideas from #277 (comment)
Now you can set a default reviver at initialization with
or you can pass a reviver function as an optional parameter to any public function that uses JSON.parse:
useMMKVStorage
useMMKVStorage(storage, 'default', {reviver: myReviver})
getMap
storage.getMap('myKey', myCallback, myReviver)
getMapAsync
storage.getMapAsync('myKey', myReviver)
getArray
storage.getArray('myKey', myCallback, myReviver)
getArrayAsync
storage.getArrayAsync('myKey', myReviver)
getMultipleItems
storage.getMultipleItems(['myKey'], 'array', myReviver)
arrayIndex.getAll
indexer.getAll(myReviver)
mapIndex.getAll
indexer.getAll(myReviver)
I also fixed some docs typos which were causing 404s:
Thanks!