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

MMKVStorageWrapper is incompatible with react-native-mmkv-storage > 0.5.6 #431

Closed
mattpetrie opened this issue Jul 27, 2021 · 12 comments · Fixed by #436
Closed

MMKVStorageWrapper is incompatible with react-native-mmkv-storage > 0.5.6 #431

mattpetrie opened this issue Jul 27, 2021 · 12 comments · Fixed by #436

Comments

@mattpetrie
Copy link

Starting with version 0.5.7, react-native-mmkv-storage introduced a change making removeItem and clearStore synchronous functions.

The MMKVStorageWrapper expects these functions to return promises, which, when used in combination with react-native-mmkv-storage 0.5.7 or higher, results in errors like this when the attempt to chain a then() onto removeItem is made:

[apollo-cache-persist] Error purging cache storage [TypeError: _this.storage.removeItem(key).then is not a function. (In '_this.storage.removeItem(key).then(function () {
          return resolve();
        })', '_this.storage.removeItem(key).then' is undefined)]

Since the wrapper's own removeItem and clearStore methods returns promises, perhaps they could first check to if the wrapped method is thenable, then resolve the promise when either the the wrapped method resolves or after invoking the synchronous method.

@wodCZ
Copy link
Collaborator

wodCZ commented Jul 27, 2021

Hi, thanks for a nice report. I haven't even noticed this change in react-native-mmkv-storage, thanks for commit link!

I should be able to prepare a PR with the fix within few days, if nobody else takes it. The approach you're suggesting sounds good to me.

wodCZ added a commit to wodCZ/apollo-cache-persist that referenced this issue Aug 1, 2021
The MMKV storage's removeItem is synchronous since 0.5.7, which caused the .then() call to fail.
This adds backwards-compatible support for both variants.

fix apollographql#431
wodCZ added a commit to wodCZ/apollo-cache-persist that referenced this issue Aug 1, 2021
The MMKV storage's removeItem is synchronous since 0.5.7, which caused the .then() call to fail.
This adds backwards-compatible support for both variants.

fix apollographql#431
wodCZ added a commit to wodCZ/apollo-cache-persist that referenced this issue Aug 2, 2021
fix apollographql#426, fix apollographql#431 by adding support for new synchronous removeItem
wodCZ added a commit to wodCZ/apollo-cache-persist that referenced this issue Aug 2, 2021
fix apollographql#426, fix apollographql#431 by adding support for new synchronous removeItem
@wodCZ wodCZ linked a pull request Aug 2, 2021 that will close this issue
@craftycorvid
Copy link

craftycorvid commented Nov 30, 2021

This is still an issue on version 0.13 of apollo-cache-persist. Tested on react-native-mmkv-storage 0.6.0 and 0.6.6

[apollo-cache-persist] Error purging cache storage [TypeError: _this.storage.removeItem(key).then is not a function. (In '_this.storage.removeItem(key).then(function () {
          return resolve();
        })', '_this.storage.removeItem(key).then' is undefined)]```

@wodCZ
Copy link
Collaborator

wodCZ commented Nov 30, 2021

@Ivan0xFF could you please attach your persistor configuration/initialization code (including imports please)?

@craftycorvid
Copy link

craftycorvid commented Nov 30, 2021

Here's the relevant bits.

import { MMKVStorageWrapper, CachePersistor } from 'apollo3-cache-persist';
import { InMemoryCache } from '@apollo/client';
import { MMKV } from '@root/hooks/app/use-storage'; // This returns the result of useMMKVStorage(key, MMKV)

const cache = new InMemoryCache({ ... });

const newPersistor = new CachePersistor({
                cache: cache,
                storage: new MMKVStorageWrapper(MMKV),
                debug: process.env.ENV !== 'prod',
                trigger: 'background'
            });
await newPersistor.restore();
            

@wodCZ
Copy link
Collaborator

wodCZ commented Nov 30, 2021

@Ivan0xFF

The storage wrapper doesn't (cannot) work with the useMMKVStorage(key, MMKV) return value, since it returns value of a single key.

You have to pass the whole MMKV instance:

import { MMKVStorageWrapper, CachePersistor } from 'apollo3-cache-persist';
import { InMemoryCache } from '@apollo/client';

const MMKV = new MMKVStorage.Loader().withInstanceID('apolloCache3').initialize();

const newPersistor = new CachePersistor({
                cache: cache,
                storage: new MMKVStorageWrapper(MMKV),
                debug: process.env.ENV !== 'prod',
                trigger: 'background'
            });
await newPersistor.restore();

@craftycorvid
Copy link

I see, thanks for clarifying. I'll give that shot!

@craftycorvid
Copy link

@wodCZ My mistake, I had misread the code. That was never returning useMKVStorage. Here's a clearer picture of how I'm initializing this and hitting this issue.

import { MMKVStorageWrapper, CachePersistor } from 'apollo3-cache-persist';
import { InMemoryCache } from '@apollo/client';
import MMKVStorage from 'react-native-mmkv-storage';

const cache = new InMemoryCache({ ... });

const MMKV: MMKVStorage.API = new MMKVStorage.Loader().initialize();

const newPersistor = new CachePersistor({
                cache: cache,
                storage: new MMKVStorageWrapper(MMKV),
                debug: process.env.ENV !== 'prod',
                trigger: 'background'
            });
await newPersistor.restore();

@wodCZ
Copy link
Collaborator

wodCZ commented Nov 30, 2021

Can you please double-check the installed versions using

npm list apollo3-cache-persist react-native-mmkv-storage

Also, the result of the following code would be helpful.

cat node_modules/apollo3-cache-persist/lib/storageWrappers/MMKVStorageWrapper.js

@craftycorvid
Copy link

craftycorvid commented Nov 30, 2021

├── apollo3-cache-persist@0.13.0 
└── react-native-mmkv-storage@0.6.6 
var MMKVStorageWrapper = (function () {
    function MMKVStorageWrapper(storage) {
        this.storage = storage;
    }
    MMKVStorageWrapper.prototype.getItem = function (key) {
        return this.storage.getItem(key) || null;
    };
    MMKVStorageWrapper.prototype.removeItem = function (key) {
        var _this = this;
        return new Promise(function (resolve, reject) {
            Promise.resolve(_this.storage.removeItem(key))
                .then(function () { return resolve(); })
                .catch(function () { return reject(); });
        });
    };
    MMKVStorageWrapper.prototype.setItem = function (key, value) {
        var _this = this;
        return new Promise(function (resolve, reject) {
            _this.storage
                .setItem(key, value)
                .then(function () { return resolve(); })
                .catch(function () { return reject(); });
        });
    };
    return MMKVStorageWrapper;
}());
export { MMKVStorageWrapper };
//# sourceMappingURL=MMKVStorageWrapper.js.map

@wodCZ
Copy link
Collaborator

wodCZ commented Nov 30, 2021

That's weird. From the code it's clear that the error you mentioned

[apollo-cache-persist] Error purging cache storage [TypeError: _this.storage.removeItem(key).then is not a function. (In '_this.storage.removeItem(key).then(function () {
          return resolve();
        })', '_this.storage.removeItem(key).then' is undefined)]```

doesn't match the actual code.

Promise.resolve(_this.storage.removeItem(key))
                .then(function () { return resolve(); })
                .catch(function () { return reject(); });

My best guess is that you're running an outdated bundle on the device/emulator.
Running yarn start --reset-cache and rebuilding the app could help.
Other than that I have no idea what could be going on, sorry.

@craftycorvid
Copy link

Yeah, I see that as well. I'll keep trying things and report back if I find a solution. Thanks for all the help!

@craftycorvid
Copy link

Ok, cache was the culprit. Running react-native start --reset-cache fixed the issue.

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