Issue #839: Re-add isMobXReactObserver property and perform check on …

…class components (#843)

* Update size limit config to 4.1

* Issue #839: Re-add isMobXReactObserver property and perform check on class components.

* Better observer class warnings, WIP

* Add __DEV__ as global in lint config

* Better logging, fix tests, and clean up

* Log redeclared observer warning in both production and development

* Fix broken tests

- Update mobx-react-lite dependency version
- Import observer batching in jest setup config

* Semantic versioning greater or equal to mobx-react-lite dependency version 2.0.6

.eslintrc.js

@@ -8,7 +8,8 @@ module.exports = {
         node: true
     },
     globals: {
-        process: "readonly"
+        process: "readonly",
+        __DEV__: "readonly"
     },
     parserOptions: {
         ecmaVersion: 6,

.size-limit.json

@@ -1,7 +1,7 @@
 [
     {
         "path": "dist/mobxreact.umd.production.min.js",
-        "limit": "4 KB",
+        "limit": "4.1 KB",
         "webpack": false,
         "running": false
     }

CHANGELOG.md

@@ -1,5 +1,9 @@
 # MobX-React Changelog
 
+### 6.2.3
+
+-   Log warning if class component is already an observer to prevent memory leaks.  [#839](https://github.com/mobxjs/mobx-react/issues/839)
+
 ### 6.2.2
 
 -   Observer batching imports are kept in production builds as side effects ([see issue](https://github.com/mobxjs/mobx-react-lite/issues/273))

jest.setup.ts

@@ -1,4 +1,5 @@
 import "@testing-library/jest-dom/extend-expect"
+import "mobx-react-lite/batchingForReactDom"
 
 // @ts-ignore
 global.__DEV__ = true

package.json

@@ -90,7 +90,7 @@
     "typescript": "^3.7.0"
   },
   "dependencies": {
-    "mobx-react-lite": "2"
+    "mobx-react-lite": ">=2.0.6"
   },
   "files": [
     "dist",

src/observer.tsx

@@ -39,7 +39,7 @@ export function observer<T extends IReactComponent>(component: T): T {
         if (typeof baseRender !== "function")
             throw new Error("render property of ForwardRef was not a function")
         return React.forwardRef(function ObserverForwardRef() {
-            const args = arguments;
+            const args = arguments
             return <Observer>{() => baseRender.apply(undefined, args)}</Observer>
         }) as T
     }

src/observerClass.ts

@@ -12,6 +12,7 @@ import { isUsingStaticRendering } from "mobx-react-lite"
 import { newSymbol, shallowEqual, setHiddenProp, patch } from "./utils/utils"
 
 const mobxAdminProperty = $mobx || "$mobx"
+const mobxObserverProperty = newSymbol("isMobXReactObserver")
 const mobxIsUnmounted = newSymbol("isUnmounted")
 const skipRenderKey = newSymbol("skipRender")
 const isForcingUpdateKey = newSymbol("isForcingUpdate")
@@ -20,6 +21,17 @@ export function makeClassComponentObserver(
     componentClass: React.ComponentClass<any, any>
 ): React.ComponentClass<any, any> {
     const target = componentClass.prototype
+
+    if (componentClass[mobxObserverProperty]) {
+        const displayName = getDisplayName(this)
+        console.warn(
+            `The provided component class (${displayName}) 
+                has already been declared as an observer component.`
+        )
+    } else {
+        componentClass[mobxObserverProperty] = true
+    }
+
     if (target.componentWillReact)
         throw new Error("The componentWillReact life-cycle event is no longer supported")
     if (componentClass["__proto__"] !== PureComponent) {
@@ -44,15 +56,18 @@ export function makeClassComponentObserver(
     }
     patch(target, "componentWillUnmount", function() {
         if (isUsingStaticRendering() === true) return
-        if (this.render[mobxAdminProperty]) {
-            this.render[mobxAdminProperty].dispose()
-        } else if (__DEV__) {
+        this.render[mobxAdminProperty]?.dispose()
+        this[mobxIsUnmounted] = true
+
+        if (!this.render[mobxAdminProperty]) {
+            // Render may have been hot-swapped and/or overriden by a subclass.
             const displayName = getDisplayName(this)
             console.warn(
-                `The render function for an observer component (${displayName}) was modified after MobX attached. This is not supported, since the new function can't be triggered by MobX.`
+                `The reactive render of an observer class component (${displayName}) 
+                was overriden after MobX attached. This may result in a memory leak if the 
+                overriden reactive render was not properly disposed.`
             )
         }
-        this[mobxIsUnmounted] = true
     })
     return componentClass
 }

test/inject.test.tsx

@@ -397,7 +397,7 @@ describe("inject based context", () => {
         let injectRender = 0
 
         function connect(): IReactComponent {
-            const args = arguments;
+            const args = arguments
             return (component: IReactComponent) =>
                 // @ts-ignore
                 inject.apply(this, args)(observer(component))

test/misc.test.tsx

@@ -2,7 +2,6 @@ import React from "react"
 import { extendObservable, observable } from "mobx"
 import { observer } from "../src"
 import { render } from "@testing-library/react"
-import { withConsole } from "./utils/withConsole"
 
 test("issue mobx 405", () => {
     function ExampleState() {
@@ -86,25 +85,3 @@ test("#85 Should handle state changing in constructors", () => {
     a.set(7)
     expect(container).toHaveTextContent("child:7 - parent:7")
 })
-
-test("Do not warn about custom shouldComponentUpdate when it is the one provided by ReactiveMixin", () => {
-    withConsole(() => {
-        const A = observer(
-            class A extends React.Component {
-                render() {
-                    return null
-                }
-            }
-        )
-
-        observer(
-            class B extends A {
-                render() {
-                    return null
-                }
-            }
-        )
-
-        expect(console.warn).not.toHaveBeenCalled()
-    })
-})

test/observer.test.tsx

@@ -845,6 +845,7 @@ test.skip("#709 - applying observer on React.memo component", () => {
 })
 
 test("#797 - replacing this.render should trigger a warning", () => {
+    const warn = jest.spyOn(global.console, "warn")
     @observer
     class Component extends React.Component {
         render() {
@@ -860,16 +861,20 @@ test("#797 - replacing this.render should trigger a warning", () => {
     const compRef = React.createRef<Component>()
     const { unmount } = render(<Component ref={compRef} />)
     compRef.current?.swapRenderFunc()
+    unmount()
 
-    const msg: Array<string> = []
-    const warn_orig = console.warn
-    console.warn = m => msg.push(m)
+    expect(warn).toHaveBeenCalled()
+})
 
-    unmount()
+test("Redeclaring an existing observer component as an observer should log a warning", () => {
+    const warn = jest.spyOn(global.console, "warn")
+    @observer
+    class AlreadyObserver extends React.Component<any, any> {
+        render() {
+            return <div />
+        }
+    }
 
-    expect(msg.length).toBe(1)
-    expect(msg[0]).toBe(
-        `The render function for an observer component (Component) was modified after MobX attached. This is not supported, since the new function can't be triggered by MobX.`
-    )
-    console.warn = warn_orig
+    observer(AlreadyObserver)
+    expect(warn).toHaveBeenCalled()
 })

yarn.lock

@@ -7189,10 +7189,10 @@ [email protected], [email protected], mkdirp@^0.5.1, mkdirp@~0.5.1:
   dependencies:
     minimist "0.0.8"
 
-mobx-react-lite@2:
-  version "2.0.0"
-  resolved "https://registry.yarnpkg.com/mobx-react-lite/-/mobx-react-lite-2.0.0.tgz#52ee837a81b4a4cd02fe4a2e6e9433da89b6f9a5"
-  integrity sha512-IrA0WR8f15hq1BLnNfgtQQ0Gp9zRASLZxXEVfiJv8uTwj1K/wN7vHAJtr9YJyBFia0W6QUAXFPP0PHdV0M/L9g==
+mobx-react-lite@>=2.0.6:
+  version "2.0.6"
+  resolved "https://registry.yarnpkg.com/mobx-react-lite/-/mobx-react-lite-2.0.6.tgz#e1307a2b271c6a6016c8ad815a25014b7f95997d"
+  integrity sha512-h/5GqxNIoSqnjt7SHxVtU7i1Kg0Xoxj853amzmzLgLRZKK9WwPc9tMuawW79ftmFSQhML0Zwt8kEuG1DIjQNBA==
 
 mobx@^5.15.4:
   version "5.15.4"