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

Allow unserialised data to be written to store #420

Merged
merged 1 commit into from
Mar 28, 2021

Conversation

PMCorbett
Copy link
Contributor

Resolves #419

This PR: #388 stops the cache being written as anything other than a string.

The CachePersistor accepts an option "serialize: boolean" that can allow for data to be written to the store without being serialised to a string first. However with the .toString() coercion, when serialize is true, all that gets written to the store is [object object], rather than the object.

This change makes sense, to bring the api inline with the web storage api. However, it means all data must be stored as JSON, which, when there is a large data set, is very expensive in memory and cpu.

So this PR resolves the issue by putting the generic back in, removing the coercion, and assigning the appropriate generic to each of the storage wrappers (I think everything is string apart from localForage, but perhaps other wrappers can support non-string values)

This PR: apollographql#388
stops the cache being written as anything other than a string.

The CachePersistor accepts an option "serialize: boolean" that can allow
for data to be written to the store without being serialised to a string
first. However with the .toString() coercion, when serialize is true,
all that gets written to the store is [object object], rather than the
object.

This change makes sense, to bring the api inline with the web storage
api. However, it means all data must be stored as JSON, which, when
there is a large data set, is very expensive in memory and cpu.

So this resolves the issue by putting the generic back in, removing the
coercion, and assigning the appropriate generic to each of the storage
wrappers (I think everything is string apart from localForage, but
perhaps other wrappers can support non-string values)
@apollo-cla
Copy link

@PMCorbett: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

getItem: (key: string) => string | null | Promise<string | null>;
setItem: (key: string, value: string) => void | Promise<void>;
removeItem: (key: string) => void | Promise<void>;
export interface PersistentStorage<T> {
Copy link
Collaborator

@wtrocki wtrocki Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be breaking change

Suggested change
export interface PersistentStorage<T> {
export interface PersistentStorage<T = any> {

@wtrocki
Copy link
Collaborator

wtrocki commented Mar 26, 2021

Changes look ok and make sense. Did you tried to run our examples to see if that works?

@wtrocki wtrocki merged commit 7bcb322 into apollographql:master Mar 28, 2021
@jimbuck
Copy link

jimbuck commented Jul 2, 2021

Any idea on when a new release is coming out? I'd love to use this but it's not on NPM yet.

@wtrocki
Copy link
Collaborator

wtrocki commented Jul 5, 2021

@jimbuck my bad. Totally forgot to release this. We do not have any automation so releases are manual.
This is released now along with other fixes

npm notice name:          apollo3-cache-persist                   
npm notice version:       0.10.0  

@jimbuck
Copy link

jimbuck commented Jul 5, 2021

No problem, thanks for getting the update out!

And just so you know, we're using custom serialization so we can revive dates correct from localstorage. I'm not sure if there is a better way to handle that but should meet our use case.

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.

Serialize: false option is not working as expected
4 participants