add warn for extending builtins (#2769)

* add warn for extending builtins * refactor: urugator code review * refactor: pass stable name for unit test
mobxjs · Feb 4, 2021 · 7820e5e · 7820e5e
1 parent a8e581e
commit 7820e5e
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 2 deletions.
diff --git a/.changeset/nasty-snails-argue.md b/.changeset/nasty-snails-argue.md
@@ -0,0 +1,5 @@
+---
+"mobx": patch
+---
+
+add warn for extending builtins
diff --git a/packages/mobx/__tests__/v5/base/make-observable.ts b/packages/mobx/__tests__/v5/base/make-observable.ts
@@ -1,4 +1,5 @@
 import { isFlow } from "../../../src/api/flow"
+import { deepEnhancer } from "../../../src/internal"
 import {
     makeObservable,
     action,
@@ -17,7 +18,8 @@ import {
     configure,
     flow,
     override,
-    $mobx
+    ObservableSet,
+    ObservableMap
 } from "../../../src/mobx"
 
 test("makeObservable picks up decorators", () => {
@@ -223,6 +225,43 @@ test("makeObservable supports autoBind", () => {
     expect(t.bound.call(undefined)).toBe(t)
 })
 
+test("Extending builtins is not support #2765", () => {
+    class ObservableMapLimitedSize extends ObservableMap {
+        limitSize = 0
+        constructor(map: Map<any, any>, enhancer = deepEnhancer, name: string) {
+            super(map, enhancer, name)
+            makeObservable(this, {
+                limitSize: observable
+            })
+        }
+    }
+
+    class ObservableSetLimitedSize extends ObservableSet {
+        limitSize = 0
+        constructor(set: Set<any>, enhancer = deepEnhancer, name: string) {
+            super(set, enhancer, name)
+            makeObservable(this, {
+                limitSize: observable
+            })
+        }
+    }
+
+    expect(() => {
+        new ObservableMapLimitedSize(new Map([["key", "val"]]), deepEnhancer, "TestObject")
+    }).toThrowErrorMatchingInlineSnapshot(`
+        "[MobX] Cannot convert 'TestObject' into observable object:
+        The target is already observable of different type.
+        Extending builtins is not supported."
+    `)
+    expect(() => {
+        new ObservableSetLimitedSize(new Set(), deepEnhancer, "TestObject")
+    }).toThrowErrorMatchingInlineSnapshot(`
+        "[MobX] Cannot convert 'TestObject' into observable object:
+        The target is already observable of different type.
+        Extending builtins is not supported."
+    `)
+})
+
 test("makeAutoObservable has sane defaults", () => {
     class Test {
         x = 3

diff --git a/packages/mobx/src/types/observableobject.ts b/packages/mobx/src/types/observableobject.ts
@@ -44,6 +44,8 @@ import {
     isOverride,
     defineProperty,
     inferAnnotationFromDescriptor,
+    getDebugName,
+    getAdministration,
     objectPrototype
 } from "../internal"
 
@@ -645,7 +647,16 @@ export function asObservableObject(
         die(`Options can't be provided for already observable objects.`)
     }
 
-    if (hasProp(target, $mobx)) return target
+    if (hasProp(target, $mobx)) {
+        if (__DEV__ && !(getAdministration(target) instanceof ObservableObjectAdministration)) {
+            die(
+                `Cannot convert '${getDebugName(target)}' into observable object:` +
+                    `\nThe target is already observable of different type.` +
+                    `\nExtending builtins is not supported.`
+            )
+        }
+        return target
+    }
 
     if (__DEV__ && !Object.isExtensible(target))
         die("Cannot make the designated object observable; it is not extensible")