From 3602357ef439ef4a659797cb201b988f229d9a09 Mon Sep 17 00:00:00 2001 From: christianalfoni Date: Fri, 28 Dec 2018 13:54:06 +0100 Subject: [PATCH 1/2] fix(proxy-state-tree): optimally remove cached proxies that has new values on their path --- .../proxy-state-tree/src/Proxyfier.ts | 106 ++++++++++++++---- .../proxy-state-tree/src/index.test.ts | 63 ++++++++++- .../proxy-state-tree/src/index.ts | 13 ++- .../proxy-state-tree/src/types.ts | 6 + 4 files changed, 167 insertions(+), 21 deletions(-) diff --git a/packages/node_modules/proxy-state-tree/src/Proxyfier.ts b/packages/node_modules/proxy-state-tree/src/Proxyfier.ts index 0159acaf..6633ba54 100644 --- a/packages/node_modules/proxy-state-tree/src/Proxyfier.ts +++ b/packages/node_modules/proxy-state-tree/src/Proxyfier.ts @@ -4,6 +4,7 @@ import isPlainObject from 'is-plain-obj' export const IS_PROXY = Symbol('IS_PROXY') export const PATH = Symbol('PATH') export const VALUE = Symbol('VALUE') +export const CACHED_PROXY = Symbol('CACHED_PROXY') const arrayMutations = new Set([ 'push', @@ -16,22 +17,77 @@ const arrayMutations = new Set([ 'copyWithin', ]) -const removeProxyMutations = ['set', 'splice', 'shift', 'unshift'] - export class Proxifier { private proxyCache = {} constructor(private tree: TTree) { - tree.master.onMutation(this.removeProxies) + tree.master.onRemoveProxy(this.removeProxies) } private concat(path, prop) { return path ? path + '.' + prop : prop } - addProxyToCache(path: string, proxy: any) { - return (this.proxyCache[path] = proxy) + const pathArray = path.split('.') + let currentCache = this.proxyCache + const length = pathArray.length + const keyIndex = length - 1 + + for (let x = 0; x < length; x++) { + const key = pathArray[x] + + if (!currentCache[key]) { + currentCache[key] = {} + } + + if (x === keyIndex) { + currentCache[key][CACHED_PROXY] = proxy + } else { + currentCache = currentCache[key] + } + } + + return proxy } + getProxyFromCache(path: string) { - return this.proxyCache[path] + const pathArray = path.split('.') + let currentCache = this.proxyCache + const length = pathArray.length + const keyIndex = length - 1 + + for (let x = 0; x < length; x++) { + const key = pathArray[x] + + if (!currentCache[key]) { + return null + } + + if (x === keyIndex) { + return currentCache[key][CACHED_PROXY] + } else { + currentCache = currentCache[key] + } + } + } + + removeProxies = (path: string) => { + const pathArray = path.split('.') + let currentCache = this.proxyCache + const length = pathArray.length + const keyIndex = length - 1 + + for (let x = 0; x < length; x++) { + const key = pathArray[x] + + if (!currentCache[key]) { + return null + } + + if (x === keyIndex) { + delete currentCache[key] + } else { + currentCache = currentCache[key] + } + } } shouldTrackMutations(path) { @@ -41,16 +97,6 @@ export class Proxifier { ) } - removeProxies = (mutation: IMutation) => { - if (removeProxyMutations.includes(mutation.method)) { - for (let path in this.proxyCache) { - if (path.indexOf(mutation.path) === 0) { - delete this.proxyCache[path] - } - } - } - } - ensureMutationTrackingIsEnabled(path) { if (this.tree.master.options.devmode && !this.tree.canMutate()) { throw new Error( @@ -142,9 +188,19 @@ export class Proxifier { proxifier.ensureMutationTrackingIsEnabled(nestedPath) return (...args) => { const mutationTree = proxifier.getMutationTree() + const method = String(prop) + + // On POP we can optimally remove cached proxy by removing the specific one + // that was removed. If it is a PUSH, we do not have to remove anything, as + // existing proxies stays the same + if (method === 'pop') { + proxifier.tree.master.removeProxy(nestedPath) + } else if (method !== 'push') { + proxifier.tree.master.removeProxy(path) + } mutationTree.addMutation({ - method: String(prop), + method, path: path, args: args, }) @@ -167,6 +223,10 @@ export class Proxifier { const mutationTree = proxifier.getMutationTree() + if (isPlainObject(target[prop])) { + proxifier.tree.master.removeProxy(nestedPath) + } + mutationTree.addMutation({ method: 'set', path: nestedPath, @@ -248,6 +308,10 @@ export class Proxifier { const mutationTree = proxifier.getMutationTree() + if (isPlainObject(target[prop]) || Array.isArray(target[prop])) { + proxifier.tree.master.removeProxy(nestedPath) + } + mutationTree.addMutation( { method: 'set', @@ -277,6 +341,10 @@ export class Proxifier { const mutationTree = proxifier.getMutationTree() + if (isPlainObject(target[prop]) || Array.isArray(target[prop])) { + proxifier.tree.master.removeProxy(nestedPath) + } + mutationTree.addMutation( { method: 'unset', @@ -295,9 +363,9 @@ export class Proxifier { ) ) } - proxify(value, path) { + proxify(value: any, path: string) { if (value) { - if (value[IS_PROXY] && value[PATH] !== path) { + if (value[IS_PROXY] && String(value[PATH]) !== String(path)) { return this.proxify(value[VALUE], path) } else if (value[IS_PROXY]) { return value diff --git a/packages/node_modules/proxy-state-tree/src/index.test.ts b/packages/node_modules/proxy-state-tree/src/index.test.ts index 26b8f533..88e3e4af 100644 --- a/packages/node_modules/proxy-state-tree/src/index.test.ts +++ b/packages/node_modules/proxy-state-tree/src/index.test.ts @@ -1,4 +1,4 @@ -import { IS_PROXY, ProxyStateTree } from './' +import { IS_PROXY, ProxyStateTree, CACHED_PROXY } from './' describe('CREATION', () => { test('should create a ProxyStateTree instance', () => { @@ -917,3 +917,64 @@ describe('GETTER', () => { expect(renderCount).toBe(2) }) }) + +describe('PROXY CACHE', () => { + it('should cache proxies', () => { + const tree = new ProxyStateTree({ + foo: { + bar: 'baz', + }, + }) + + const accessTree = tree.getTrackStateTree() + + const proxy = accessTree.state.foo + const proxifier = accessTree.proxifier as any + expect(proxifier.proxyCache.foo[CACHED_PROXY]).toBe(proxy) + }) + + it('should replace cached proxies when path mutated', () => { + const tree = new ProxyStateTree({ + foo: { + bar: 'baz', + }, + }) + + const accessTree = tree.getTrackStateTree() + const mutateTree = tree.getMutationTree() + + const proxy = accessTree.state.foo + + mutateTree.state.foo = { bar: 'baz2' } + + const proxy2 = accessTree.state.foo + + const proxifier = accessTree.proxifier as any + expect(proxifier.proxyCache.foo[CACHED_PROXY]).not.toBe(proxy) + expect(proxifier.proxyCache.foo[CACHED_PROXY]).toBe(proxy2) + }) + it('should remove cached proxies across proxifiers', () => { + const tree = new ProxyStateTree({ + foo: { + bar: 'baz', + }, + }) + + const mutateTree = tree.getMutationTree() + const mutateTree2 = tree.getMutationTree() + + const proxy = mutateTree.state.foo + const proxy2 = mutateTree2.state.foo + + const proxifier = mutateTree.proxifier as any + const proxifier2 = mutateTree2.proxifier as any + + expect(proxifier.proxyCache.foo[CACHED_PROXY]).toBe(proxy) + expect(proxifier2.proxyCache.foo[CACHED_PROXY]).toBe(proxy2) + + delete mutateTree.state.foo + + expect(proxifier.proxyCache.foo).toBe(undefined) + expect(proxifier2.proxyCache.foo).toBe(undefined) + }) +}) diff --git a/packages/node_modules/proxy-state-tree/src/index.ts b/packages/node_modules/proxy-state-tree/src/index.ts index bd2e35ea..74eb41a7 100644 --- a/packages/node_modules/proxy-state-tree/src/index.ts +++ b/packages/node_modules/proxy-state-tree/src/index.ts @@ -1,4 +1,4 @@ -import { IS_PROXY, VALUE, PATH, Proxifier } from './Proxyfier' +import { IS_PROXY, VALUE, PATH, Proxifier, CACHED_PROXY } from './Proxyfier' import isPlainObject from 'is-plain-obj' import { IMutation, @@ -11,6 +11,7 @@ import { IProxyStateTree, IFlushCallback, IProxifier, + IRemoveProxyCallback, } from './types' import { TrackMutationTree } from './TrackMutationTree' import { TrackStateTree } from './TrackStateTree' @@ -20,6 +21,7 @@ export { IS_PROXY, VALUE, PATH, + CACHED_PROXY, IMutation, ITrackCallback, ITrackStateTree, @@ -32,6 +34,7 @@ export * from './types' export class ProxyStateTree implements IProxyStateTree { private mutationCallbacks: IMutationCallback[] = [] + private removeProxyCallbacks: IRemoveProxyCallback[] = [] private flushCallbacks: IFlushCallback[] = [] private cache = { trackMutationTree: [] as ITrackMutationTree[], @@ -82,6 +85,14 @@ export class ProxyStateTree implements IProxyStateTree { '' ) } + onRemoveProxy(cb: IRemoveProxyCallback) { + this.removeProxyCallbacks.push(cb) + } + removeProxy(path: string) { + for (let cb of this.removeProxyCallbacks) { + cb(path) + } + } getMutationTree(): ITrackMutationTree { if (IS_PRODUCTION) { return (this.mutationTree = diff --git a/packages/node_modules/proxy-state-tree/src/types.ts b/packages/node_modules/proxy-state-tree/src/types.ts index 50d52c70..25ff1f6c 100644 --- a/packages/node_modules/proxy-state-tree/src/types.ts +++ b/packages/node_modules/proxy-state-tree/src/types.ts @@ -67,6 +67,10 @@ export interface IFlushCallback { export type TTree = ITrackMutationTree | ITrackStateTree +export interface IRemoveProxyCallback { + (path: string): void +} + export interface IProxyStateTree { addMutation(mutation: IMutation, objectChangePath?: string): void addPathDependency(path: string, callback: ITrackCallback): void @@ -76,6 +80,8 @@ export interface IProxyStateTree { changeTrackStateTree(tree: ITrackStateTree): void disposeTree(proxy: TTree): void onMutation(cb: IMutationCallback): void + onRemoveProxy(cb: IRemoveProxyCallback): void + removeProxy(path: string): void onFlush(cb: IFlushCallback): void rescope(value: any, tree: TTree): any sourceState: T From 2d8a84503bd3234147ecde627f2903e9fca2470f Mon Sep 17 00:00:00 2001 From: christianalfoni Date: Fri, 28 Dec 2018 13:54:31 +0100 Subject: [PATCH 2/2] fix(overmind): correctly pass operatorId on last flush of operators --- packages/node_modules/overmind/src/index.ts | 22 ++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/node_modules/overmind/src/index.ts b/packages/node_modules/overmind/src/index.ts index e571f09d..9b414380 100644 --- a/packages/node_modules/overmind/src/index.ts +++ b/packages/node_modules/overmind/src/index.ts @@ -246,10 +246,10 @@ export class Overmind implements Configuration { this.trackEffects(this.effects, execution) ), (err, finalContext) => { - this.eventHub.emit( - EventType.ACTION_END, - finalContext.execution - ) + this.eventHub.emit(EventType.ACTION_END, { + ...finalContext.execution, + operatorId: finalContext.execution.operatorId - 1, + }) if (err) reject(err) else resolve(finalContext.value) } @@ -767,9 +767,21 @@ export function action( mutationTree.onMutation((mutation) => { context.execution.emit(EventType.MUTATIONS, { ...context.execution, - operatorId: context.execution.operatorId, mutations: makeStringifySafeMutations([mutation]), }) + setTimeout(() => { + const flushData = context.proxyStateTree.flush(true) + if (flushData.mutations.length) { + context.execution.send({ + type: 'flush', + data: { + ...context.execution, + ...flushData, + mutations: makeStringifySafeMutations(flushData.mutations), + }, + }) + } + }) }) } const maybePromise: any = operation({