Add onChange callback in cache configuration #217
Conversation
… actually lost the class prototype. this will make it clear that it is actually not a class
…dux state are in-sync
src/CacheSnapshot.ts
Outdated
| @@ -4,13 +4,11 @@ import { OptimisticUpdateQueue } from './OptimisticUpdateQueue'; | |||
| /** | |||
| * Maintains an immutable, point-in-time view of the cache. | |||
| */ | |||
| export class CacheSnapshot { | |||
There was a problem hiding this comment.
👎 keep the class: It helps enforce that we always construct objects with the same shape (e.g. the same set of properties, constructed in the same order). Makes bookkeeping in the VM lighter. It also aids in debugging by tagging these objects with their type
See https://jsperf.com/class-vs-object-literal-creation also
There was a problem hiding this comment.
Talk offline.... keep the class add comment to make sure we keep it and remove spread operator
src/Cache.ts
Outdated
| @@ -120,6 +124,10 @@ export class Cache implements Queryable { | |||
| const { snapshot, editedNodeIds } = transaction.commit(); | |||
| this._setSnapshot(snapshot, editedNodeIds); | |||
|
|
|||
| // Call the on-change callback here to notify any change | |||
| if (this._context.onChange && typeof this._context.onChange === 'function') { | |||
There was a problem hiding this comment.
You can drop the initial check and just if (typeof this._context.onChange === 'function') {
There was a problem hiding this comment.
Or, actually - just go with if (this._context.onChange) { - we already enforce that onChange is a function via types (and could do a runtime check when constructing the cache if we want to be paranoid)
There was a problem hiding this comment.
Errr fright the typeof should guard for that... yea I was worry about user in JS lib who doesn't have types to pass garbage in
There was a problem hiding this comment.
Actually if we gonna do runtime check we should do for others callback as well so it is much better to do when constructing...I agree now that there shouldn't be this runtime typecheck floating all over and just aggregate at one location
Open a separate issue -> #220
src/apollo/Hermes.ts
Outdated
| @@ -54,4 +55,8 @@ export class Hermes extends ApolloQueryable implements ApolloCache<GraphSnapshot | |||
| const query = toQuery(options.query, options.variables, options.rootId); | |||
| return this._queryable.watch(query, options.callback); | |||
| } | |||
|
|
|||
| getCacheSnapshot(): CacheSnapshot { | |||
There was a problem hiding this comment.
currentCacheSnapshot IMO - will help communicate that it frequently changes
src/context/CacheContext.ts
Outdated
| @@ -18,6 +19,8 @@ export namespace CacheContext { | |||
| export type EntityIdMapper = (node: JsonObject) => string | number | undefined; | |||
| export type EntityTransformer = (node: JsonObject) => void; | |||
| export type LogEmitter = (message: string, ...metadata: any[]) => void; | |||
| export type OnChangeCallBack = (newCacheShapshot: CacheSnapshot, editedNodeIds: Set<String>) => void; | |||
There was a problem hiding this comment.
nit: ChangeCallback (callback is one word)
src/Cache.ts
Outdated
| @@ -120,6 +124,10 @@ export class Cache implements Queryable { | |||
| const { snapshot, editedNodeIds } = transaction.commit(); | |||
| this._setSnapshot(snapshot, editedNodeIds); | |||
|
|
|||
| // Call the on-change callback here to notify any change | |||
| if (this._context.onChange && typeof this._context.onChange === 'function') { | |||
| this._context.onChange(this._snapshot, editedNodeIds); | |||
There was a problem hiding this comment.
We update the snapshot in other cases (reset()). It would probably be clearer to move this logic into _setSnapshot
There was a problem hiding this comment.
agree... forgot about the reset operation
…ation we actually lost the class prototype. this will make it clear that it is actually not a class" This reverts commit e105040.
Add onChange to cache configuration API to allow users to perform tasks such as dispatch redux action when there is a change in the cache or log the update etc.