Fix Storage API types #388
Conversation
| @@ -20,7 +20,7 @@ export default class Storage<T> { | |||
| } | |||
|
|
|||
| async write(data: PersistedData<T>): Promise<void> { | |||
| await this.storage.setItem(this.key, data); | |||
| await this.storage.setItem(this.key, data.toString()); | |||
There was a problem hiding this comment.
Sole code change here 👀.
data here is of type PersistedData<T> = string | T, giving us two options:
- In the case it's
stringthen it has atoStringmethod, and calling this leaves the string unchanged (no-op). - In the case it's
Tthen this seems to always be inferred asNormalizedCacheObject, which is an object whosetoStringwould yield[object Object], but where relying on implicit type conversion withinlocalStoragewould (I believe) give the same result of[object Object](also no-op?).
@wtrocki, I took a look at this with an eye to use a wrapper as you suggested, but I actually couldn't find any reason (based on the code as-is) why |
dchambers
force-pushed
the
fix-storage-api-types
branch
from
7677fff to
356c653
Compare
dchambers
force-pushed
the
fix-storage-api-types
branch
from
356c653 to
d5f016c
Compare
| getItem: (key: string) => Promise<T | null> | T | null; | ||
| setItem: (key: string, data: T) => Promise<T> | Promise<void> | void | T; | ||
| removeItem: (key: string) => Promise<T> | Promise<void> | void; | ||
| export interface PersistentStorage { |
There was a problem hiding this comment.
The definitions within PersistentStorage were updated to match TypeScript's internal types for the Storage interface, but then amended to also support asynchronous storage APIs too, as needed by AsyncStorage, localForage and Ionic Storage.
There was a problem hiding this comment.
Amazing work @dchambers!
This was outstanding for very very long time.
I might need to leave it for a while before releasing for the test purposes, but all sounds good!
|
apollo3-cache-persist (0.8.0) |
|
This PR broke assignment of localForage, see #399. |
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.
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)
Alignment of
PersistentStoragewith Typescript's internal types for theStorageinterface of the Web Storage API (potential fix for #55).This change removes the generic type
TfromPersistentStorage, which seemed unnecessary and unused. Apart from some type changes there's a single code change withinsrc/Storage.tswhich will want some scrutiny from those who understand the code better.