Merge pull request cerebral#175 from cerebral/optimizeProxyRemoval

Optimize proxy removal

Author: christianalfoni

packages/node_modules/overmind/src/index.ts

@@ -246,10 +246,10 @@ export class Overmind<Config extends Configuration> 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<Input, Config extends Configuration = TheConfig>(
         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({

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

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)
+  })
+})

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<T extends object> implements IProxyStateTree<T> {
   private mutationCallbacks: IMutationCallback[] = []
+  private removeProxyCallbacks: IRemoveProxyCallback[] = []
   private flushCallbacks: IFlushCallback[] = []
   private cache = {
     trackMutationTree: [] as ITrackMutationTree<T>[],
@@ -82,6 +85,14 @@ export class ProxyStateTree<T extends object> implements IProxyStateTree<T> {
       ''
     )
   }
+  onRemoveProxy(cb: IRemoveProxyCallback) {
+    this.removeProxyCallbacks.push(cb)
+  }
+  removeProxy(path: string) {
+    for (let cb of this.removeProxyCallbacks) {
+      cb(path)
+    }
+  }
   getMutationTree(): ITrackMutationTree<T> {
     if (IS_PRODUCTION) {
       return (this.mutationTree =

packages/node_modules/proxy-state-tree/src/types.ts

@@ -67,6 +67,10 @@ export interface IFlushCallback {
 
 export type TTree = ITrackMutationTree<any> | ITrackStateTree<any>
 
+export interface IRemoveProxyCallback {
+  (path: string): void
+}
+
 export interface IProxyStateTree<T extends object> {
   addMutation(mutation: IMutation, objectChangePath?: string): void
   addPathDependency(path: string, callback: ITrackCallback): void
@@ -76,6 +80,8 @@ export interface IProxyStateTree<T extends object> {
   changeTrackStateTree(tree: ITrackStateTree<T>): 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