fixing inheritance patching

src/disposeOnUnmount.js

@@ -6,7 +6,7 @@ const storeKey = newSymbol("disposeOnUnmount")
 function runDisposersOnWillUnmount() {
     if (!this[storeKey]) {
         // when disposeOnUnmount is only set to some instances of a component it will still patch the prototype
-        return;
+        return
     }
     this[storeKey].forEach(propKeyOrFunction => {
         const prop =
@@ -46,7 +46,7 @@ export function disposeOnUnmount(target, propertyKeyOrFunction) {
 
     // tweak the component class componentWillUnmount if not done already
     if (!componentWasAlreadyModified) {
-        patch(target, "componentWillUnmount", runDisposersOnWillUnmount, false)
+        patch(target, "componentWillUnmount", runDisposersOnWillUnmount)
     }
 
     // return the disposer as is if invoked as a non decorator

src/observer.js

@@ -91,8 +91,8 @@ export const errorsReporter = new EventEmitter()
  * Utilities
  */
 
-function patch(target, funcName, runMixinFirst = false) {
-    newPatch(target, funcName, reactiveMixin[funcName], runMixinFirst)
+function patch(target, funcName) {
+    newPatch(target, funcName, reactiveMixin[funcName])
 }
 
 function shallowEqual(objA, objB) {

src/utils/utils.js

@@ -15,100 +15,94 @@ export function newSymbol(name) {
 }
 
 const mobxMixins = newSymbol("patchMixins")
-const mobxMixin = newSymbol("patchMixin")
+const mobxPatchedDefinition = newSymbol("patchedDefinition")
+const mobxRealMethod = newSymbol("patchRealMethod")
 
-function getCreateMixins(target, methodName) {
+function getMixins(target, methodName) {
     const mixins = (target[mobxMixins] = target[mobxMixins] || {})
     const methodMixins = (mixins[methodName] = mixins[methodName] || {})
-    methodMixins.pre = methodMixins.pre || []
-    methodMixins.post = methodMixins.post || []
+    methodMixins.locks = methodMixins.locks || 0
+    methodMixins.methods = methodMixins.methods || []
     return methodMixins
 }
 
-function getMixins(target, methodName) {
-    return target[mobxMixins][methodName]
-}
-
-const cachedDefinitions = {}
-
-function createOrGetCachedDefinition(methodName, enumerable) {
-    const cacheKey = `${methodName}+${enumerable}`
-    const cached = cachedDefinitions[cacheKey]
-    if (cached) {
-        return cached
-    }
-
-    const wrapperMethod = function wrapperMethod(...args) {
-        const mixins = getMixins(this, methodName)
-
-        // avoid possible recursive calls by custom patches
-        if (mixins.realRunning) {
-            return
-        }
-        mixins.realRunning = true
+function wrapper(realMethod, mixins, ...args) {
+    // locks are used to ensure that mixins are invoked only once per invocation, even on recursive calls
+    mixins.locks++
 
-        const realMethod = mixins.real
+    try {
         let retVal
+        if (realMethod !== undefined && realMethod !== null) {
+            retVal = realMethod.apply(this, args)
+        }
 
-        try {
-            mixins.pre.forEach(pre => {
-                pre.apply(this, args)
-            })
-
-            if (realMethod !== undefined && realMethod !== null) {
-                retVal = realMethod.apply(this, args)
-            }
-
-            mixins.post.forEach(post => {
-                post.apply(this, args)
+        return retVal
+    } finally {
+        mixins.locks--
+        if (mixins.locks === 0) {
+            mixins.methods.forEach(mx => {
+                mx.apply(this, args)
             })
-
-            return retVal
-        } finally {
-            mixins.realRunning = false
         }
     }
-    wrapperMethod[mobxMixin] = true
+}
 
-    const newDefinition = {
-        get() {
-            return wrapperMethod
-        },
-        set(value) {
-            const mixins = getMixins(this, methodName)
-            mixins.real = value
-        },
-        configurable: true,
-        enumerable: enumerable
+function wrapFunction(mixins) {
+    const fn = function(...args) {
+        wrapper.call(this, fn[mobxRealMethod], mixins, ...args)
     }
-
-    cachedDefinitions[cacheKey] = newDefinition
-
-    return newDefinition
+    return fn
 }
 
-export function patch(target, methodName, mixinMethod, runMixinFirst = false) {
-    const mixins = getCreateMixins(target, methodName)
+export function patch(target, methodName, ...mixinMethods) {
+    const mixins = getMixins(target, methodName)
 
-    if (runMixinFirst) {
-        mixins.pre.unshift(mixinMethod)
-    } else {
-        mixins.post.push(mixinMethod)
+    for (const mixinMethod of mixinMethods) {
+        if (mixins.methods.indexOf(mixinMethod) < 0) {
+            mixins.methods.push(mixinMethod)
+        }
     }
 
-    const realMethod = target[methodName]
-    if (typeof realMethod === "function" && realMethod[mobxMixin]) {
-        // already patched, do not repatch
+    const oldDefinition = Object.getOwnPropertyDescriptor(target, methodName)
+    if (oldDefinition && oldDefinition[mobxPatchedDefinition]) {
+        // already patched definition, do not repatch
         return
     }
 
-    mixins.real = realMethod
-
-    const oldDefinition = Object.getOwnPropertyDescriptor(target, methodName)
-    const newDefinition = createOrGetCachedDefinition(
+    const originalMethod = target[methodName]
+    const newDefinition = createDefinition(
+        target,
         methodName,
-        oldDefinition ? oldDefinition.enumerable : undefined
+        oldDefinition ? oldDefinition.enumerable : undefined,
+        mixins,
+        originalMethod
     )
 
     Object.defineProperty(target, methodName, newDefinition)
 }
+
+function createDefinition(target, methodName, enumerable, mixins, originalMethod) {
+    const wrappedFunc = wrapFunction(mixins)
+    wrappedFunc[mobxRealMethod] = originalMethod
+
+    return {
+        [mobxPatchedDefinition]: true,
+        get: function() {
+            return wrappedFunc
+        },
+        set: function(value) {
+            if (this === target) {
+                wrappedFunc[mobxRealMethod] = value
+            } else {
+                // when it is an instance of the prototype/a child prototype patch that particular case again separately
+                // since we need to store separate values depending on wether it is the actual instance, the prototype, etc
+                // e.g. the method for super might not be the same as the method for the prototype which might be not the same
+                // as the method for the instance
+                const newDefinition = createDefinition(this, methodName, enumerable, mixins, value)
+                Object.defineProperty(this, methodName, newDefinition)
+            }
+        },
+        configurable: true,
+        enumerable: enumerable
+    }
+}

test/disposeOnUnmount.test.js

@@ -17,8 +17,8 @@ async function testComponent(C, afterMount, afterUnmount) {
 
     await asyncReactDOMRender(null, testRoot)
 
-    expect(cref.methodA).toHaveBeenCalled()
-    expect(cref.methodB).toHaveBeenCalled()
+    expect(cref.methodA).toHaveBeenCalledTimes(1)
+    expect(cref.methodB).toHaveBeenCalledTimes(1)
     if (afterUnmount) {
         afterUnmount(cref)
     }
@@ -134,8 +134,8 @@ describe("without observer", () => {
                 expect(methodD).not.toHaveBeenCalled()
             },
             () => {
-                expect(methodC).toHaveBeenCalled()
-                expect(methodD).toHaveBeenCalled()
+                expect(methodC).toHaveBeenCalledTimes(1)
+                expect(methodD).toHaveBeenCalledTimes(1)
             }
         )
     })
@@ -256,8 +256,8 @@ describe("with observer", () => {
                 expect(methodD).not.toHaveBeenCalled()
             },
             () => {
-                expect(methodC).toHaveBeenCalled()
-                expect(methodD).toHaveBeenCalled()
+                expect(methodC).toHaveBeenCalledTimes(1)
+                expect(methodD).toHaveBeenCalledTimes(1)
             }
         )
     })
@@ -347,8 +347,91 @@ test("custom patching should work", async () => {
     )
 })
 
+describe("super calls should work", async () => {
+    async function doTest(baseObserver, cObserver) {
+        const events = []
+
+        const sharedMethod = jest.fn()
+
+        class BaseComponent extends React.Component {
+            @disposeOnUnmount
+            method0 = sharedMethod
+
+            @disposeOnUnmount
+            methodA = jest.fn()
+
+            componentDidMount() {
+                events.push("baseDidMount")
+            }
+
+            componentWillUnmount() {
+                events.push("baseWillUnmount")
+            }
+        }
+
+        class C extends BaseComponent {
+            @disposeOnUnmount
+            method0 = sharedMethod
+
+            @disposeOnUnmount
+            methodB = jest.fn()
+
+            componentDidMount() {
+                super.componentDidMount()
+                events.push("CDidMount")
+            }
+
+            componentWillUnmount() {
+                super.componentWillUnmount()
+                events.push("CWillUnmount")
+            }
+
+            render() {
+                return null
+            }
+        }
+
+        if (baseObserver) {
+            BaseComponent = observer(BaseComponent)
+        }
+        if (cObserver) {
+            C = observer(C)
+        }
+
+        await testComponent(
+            C,
+            ref => {
+                expect(events).toEqual(["baseDidMount", "CDidMount"])
+                expect(sharedMethod).toHaveBeenCalledTimes(0)
+            },
+            ref => {
+                expect(events).toEqual([
+                    "baseDidMount",
+                    "CDidMount",
+                    "baseWillUnmount",
+                    "CWillUnmount"
+                ])
+                expect(sharedMethod).toHaveBeenCalledTimes(2)
+            }
+        )
+    }
+
+    it("none is observer", async () => {
+        await doTest(false, false)
+    })
+    it("base is observer", async () => {
+        await doTest(true, false)
+    })
+    it("C is observer", async () => {
+        await doTest(false, true)
+    })
+    it("both observers", async () => {
+        await doTest(true, true)
+    })
+})
+
 it("componentDidMount should be different between components", async () => {
-    async function test(withObserver) {
+    async function doTest(withObserver) {
         const events = []
 
         class A extends React.Component {
@@ -417,6 +500,6 @@ it("componentDidMount should be different between components", async () => {
         expect(events).toEqual(["mountA", "unmountA", "mountB", "unmountB"])
     }
 
-    await test(true)
-    await test(false)
+    await doTest(true)
+    await doTest(false)
 })