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

feat: add reviver function to JSON.parse calls #279

Closed
wants to merge 7 commits into from

Conversation

douugdev
Copy link
Contributor

@douugdev douugdev commented Aug 18, 2022

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

const myReviver = (key, value) => key === 'foo' ? 'bar' : value;
const storage = new MMKVLoader().withDefaultReviver(myReviver).initialize();

or you can pass a reviver function as an optional parameter to any public function that uses JSON.parse:

Function Usage
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:
image

Thanks!

@vercel
Copy link

vercel bot commented Aug 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
rnmmkv ✅ Ready (Inspect) Visit Preview Aug 18, 2022 at 10:05PM (UTC)

@douugdev douugdev changed the title Add reviver function feature feat: add reviver function to JSON.parse calls Aug 18, 2022
@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Aug 19, 2022

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.

In case of getting values in bulk from indexer transaction manager does not work as it's not implemented there but it works for getting single values. Although that could be implemented easily. If you want to do it let me know.

The fixes you made to the docs could be part of a separate PR that addresses that issue specifically.

Let me know your thoughts.

@douugdev
Copy link
Contributor Author

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 JSON.parse works and just global revivers could introduce some problems on specific scenarios:

// 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 indexer transaction manager.

I'll be closing this PR since even if you find this change relevant, I'll have to rewrite most of the code.

@douugdev douugdev closed this Aug 19, 2022
@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Aug 19, 2022

Since the mutator functions need to return a result synchronously there can't be multiple functions registered in multiple places. Transaction manager expects that you always register mutator functions outside your components only once in your app lifecycle. The best place is somewhere near where you initialize MMKV instance in a file such as storage.ts.

You can use switch on key or some other type of filtering inside the mutator functions to handle each key that you need to change on the way out/in.

// 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 array.includes returns false.

@douugdev
Copy link
Contributor Author

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?

@ammarahm-ed
Copy link
Owner

Yes, you can work on it. Remember that registered keys will be bound to each type of transaction which is similar to how mutator functions are kept.

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

Successfully merging this pull request may close these issues.

[Bug/Missing Feature] object/map storage doesn't provide reviver function
2 participants