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

fix(types): provide simplistic storage interfaces #436

Merged
merged 4 commits into from
Aug 10, 2021

Conversation

wodCZ
Copy link
Collaborator

@wodCZ wodCZ commented Aug 1, 2021

Provide an interface for each wrapper, which captures the required subset of the actual expected storage.
Also, added support for MMKV storage v0.5.7+ in the process.

fix #426, #431

@wodCZ
Copy link
Collaborator Author

wodCZ commented Aug 1, 2021

The web example with LocalStorageWrapper now works fine. I haven't tested the other storages, but since the change is the same for all the providers, I expect it'll fix all of them.

I experimented with building upon the generics, which could actually perform the type-check against the provided storage, but I failed 😔 . Maybe someone else could attempt, as the types are practically blind now.

@wtrocki
Copy link
Collaborator

wtrocki commented Aug 1, 2021

Left comments on other PR..What is the relation between them

fix apollographql#426, fix apollographql#431 by adding support for new synchronous removeItem
@wodCZ wodCZ force-pushed the fix/storage-wrapper-typings branch from 3f30b4b to 56b1c55 Compare August 2, 2021 17:28
@wodCZ
Copy link
Collaborator Author

wodCZ commented Aug 2, 2021

@wtrocki uploaded the changes as discussed in #437

This also includes the MMKV compatibility fix, since I needed to match the actual interface (the change is already bringing fruits :D ).

I haven't tested this, so please if anyone can give it a try - I'll not be able to test at least until tomorrow, but at the same time, I didn't want to stall this. LocalStorage should be simple to test using the web example, the I'd like to test at least LocalForage since it's slightly unusual & AsyncStorage as a popular RN choice.

@wodCZ wodCZ changed the title fix(types): loosen wrapper type definitions fix(types): provide simplistic storage interfaces Aug 2, 2021
@wodCZ wodCZ linked an issue Aug 2, 2021 that may be closed by this pull request
@wtrocki
Copy link
Collaborator

wtrocki commented Aug 9, 2021

Ok to merge this? Does that compile for web example?
We can do prerelease for testing purposes.

@wodCZ
Copy link
Collaborator Author

wodCZ commented Aug 10, 2021

Sorry, just got to test this.

Unfortunately, this doesn't fix the issue 😢 .

The problem is in the generics - we only type-check what the storage wrapper can accept. But the rest of the persistor is still passing around PersistedData<NormalizedCacheObject>, which of course is not assignable to string the storage accepts.

What needs to be done is that based on the serialize?: boolean option the CachePersistor must switch between "I'm passing around PersistedData<NormalizedCacheObject>" vs "I'm passing around string".

I've updated most of the types now to carry the information, but I'm missing some bit somewhere...

Not ready to merge, sorry.

@wodCZ
Copy link
Collaborator Author

wodCZ commented Aug 10, 2021

Ok, I think I fixed it.

Tested both web and react-native examples and all seems to be working.

We can do prerelease for testing purposes.

@wtrocki Please do if you approve my changes - I'm ok with merging this now.

@wtrocki wtrocki merged commit 566ebde into apollographql:master Aug 10, 2021
@wtrocki
Copy link
Collaborator

wtrocki commented Aug 10, 2021

Verified web only. Looks like it is working

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