Some cleanup and better handling of sort() and reverse()

notes.md

@@ -101,6 +101,9 @@ PHilosophy: one way to do things
 -   [ ] Breaking: in computed, the when predicate, and reaction predicate it is never allowed to directly change state. State changes should be wrapped in action.
 -   [ ] Breaking: `toJS` no longer takes action, and no longer converts Maps and Sets to plain data structures. Serializing data structures is out of scope for the MobX project, and write custom serialization methods are much more sustainable. You might even leverage @computed to serialize state.
 -   [ ] Breaking: directly calling .get() / .set() on an observable array is no longer supported
+-   [ ] Breaking: killed IObservableObject interface.
+-   [ ] Breaking: sorting or reversing an array in an derivation will now throw rather than warn.
+-   [ ] Breaking: sorting or reversing an array in an actino will no sort or reverse the source array rather than shallow copy.
 
 ## NOTES
 

src/core/action.ts

@@ -16,13 +16,21 @@ import {
     die,
     getDescriptor
 } from "../internal"
+import { addHiddenProp } from "../utils/utils"
 
 // we don't use globalState for these in order to avoid possible issues with multiple
 // mobx versions
 let currentActionId = 0
 let nextActionId = 1
-const functionNameDescriptor = getDescriptor(() => {}, "name")
-const isFunctionNameConfigurable = functionNameDescriptor?.configurable ?? false
+const isFunctionNameConfigurable = getDescriptor(() => {}, "name")?.configurable ?? false
+
+// we can safely recycle this object
+const tmpNameDescriptor: PropertyDescriptor = {
+    value: "action",
+    configurable: true,
+    writable: false,
+    enumerable: false
+}
 
 export function createAction(
     actionName: string,
@@ -38,11 +46,12 @@ export function createAction(
     function res() {
         return executeAction(actionName, autoAction, fn, ref || this, arguments)
     }
-    // TODO: can be optimized by recyclig objects? // TODO: and check if fn.name !== actionName
-    return Object.defineProperties(res, {
-        ...(isFunctionNameConfigurable && { name: { value: actionName } }),
-        isMobxAction: { value: true }
-    })
+    res.isMobxAction = true
+    if (isFunctionNameConfigurable) {
+        tmpNameDescriptor.value = actionName
+        Object.defineProperty(res, "name", tmpNameDescriptor)
+    }
+    return res
 }
 
 export function executeAction(

src/errors.ts

@@ -65,7 +65,10 @@ const niceErrors = {
         return `[ComputedValue '${name}'] It is not possible to assign a new value to a computed value.`
     },
     35: "There are multiple, different versions of MobX active. Make sure MobX is loaded only once or use `configure({ isolateGlobalState: true })`",
-    36: "isolateGlobalState should be called before MobX is running any reactions"
+    36: "isolateGlobalState should be called before MobX is running any reactions",
+    37(method) {
+        return `[mobx] \`observableArray.${method}()\` mutates the array in-place, which is not allowed inside a derivation. Use \`array.slice().${method}()\` instead`
+    }
 } as const
 
 const errors: typeof niceErrors = __DEV__ ? niceErrors : ({} as any)

src/types/dynamicobject.ts

@@ -27,7 +27,7 @@ const objectProxyTraps: ProxyHandler<any> = {
         if (isStringish(name)) return adm.has_(name)
         return (name as any) in target
     },
-    // TODO: in has / get support a `toJSON for better representation in console / debugger?
+    // TODO: support chrome formatter https://www.mattzeunert.com/2016/02/19/custom-chrome-devtools-object-formatters.html
     get(target: IIsObservableObject, name: PropertyKey) {
         if (name === $mobx || name === "constructor") return target[name]
         const adm = getAdm(target)

src/types/intercept-utils.ts

@@ -4,7 +4,6 @@ export type IInterceptor<T> = (change: T) => T | null
 
 export interface IInterceptable<T> {
     interceptors_: IInterceptor<T>[] | undefined
-    intercept_(handler: IInterceptor<T>): Lambda
 }
 
 export function hasInterceptors(interceptable: IInterceptable<any>) {

src/types/listen-utils.ts

@@ -2,7 +2,6 @@ import { Lambda, once, untrackedEnd, untrackedStart } from "../internal"
 
 export interface IListenable {
     changeListeners_: Function[] | undefined
-    observe_(handler: (change: any, oldValue?: any) => void, fireImmediately?: boolean): Lambda
 }
 
 export function hasListeners(listenable: IListenable) {

src/types/modifiers.ts

@@ -31,7 +31,6 @@ export function deepEnhancer(v, _, name) {
 
 export function shallowEnhancer(v, _, name): any {
     if (v === undefined || v === null) return v
-    // TODO: lets make sure isObservableObject become a bit smarter by setting some flag or something...
     if (isObservableObject(v) || isObservableArray(v) || isObservableMap(v) || isObservableSet(v))
         return v
     if (Array.isArray(v)) return observable.array(v, { name, deep: false })

src/types/observablearray.ts

@@ -29,6 +29,7 @@ import {
     hasProp,
     die
 } from "../internal"
+import { globalState } from "../core/globalstate"
 
 const SPLICE = "splice"
 export const UPDATE = "update"
@@ -144,7 +145,6 @@ export class ObservableArrayAdministration
         return values
     }
 
-    // TODO: rename
     intercept_(handler: IInterceptor<IArrayWillChange<any> | IArrayWillSplice<any>>): Lambda {
         return registerInterceptor<IArrayWillChange<any> | IArrayWillSplice<any>>(this, handler)
     }
@@ -415,26 +415,23 @@ export var arrayExtensions = {
         reverse(): any[] {
             // reverse by default mutates in place before returning the result
             // which makes it both a 'derivation' and a 'mutation'.
-            // so we deviate from the default and just make it an dervitation
-            if (__DEV__) {
-                console.warn(
-                    "[mobx] `observableArray.reverse()` will not update the array in place. Use `observableArray.slice().reverse()` to suppress this warning and perform the operation on a copy, or `observableArray.replace(observableArray.slice().reverse())` to reverse & update in place"
-                )
+            if (globalState.trackingDerivation) {
+                die(37, "reverse")
             }
-            const clone = (<any>this).slice()
-            return clone.reverse.apply(clone, arguments)
+            this.replace(this.slice().reverse())
+            return this
         },
 
         sort(): any[] {
             // sort by default mutates in place before returning the result
             // which goes against all good practices. Let's not change the array in place!
-            if (__DEV__) {
-                console.warn(
-                    "[mobx] `observableArray.sort()` will not update the array in place. Use `observableArray.slice().sort()` to suppress this warning and perform the operation on a copy, or `observableArray.replace(observableArray.slice().sort())` to sort & update in place"
-                )
+            if (globalState.trackingDerivation) {
+                die(37, "sort")
             }
-            const clone = (<any>this).slice()
-            return this.slice().sort.apply(clone, arguments)
+            const copy = this.slice()
+            copy.sort.apply(copy, arguments)
+            this.replace(copy)
+            return this
         },
 
         remove(value: any): boolean {

src/types/observablemap.ts

@@ -83,7 +83,6 @@ export type IObservableMapInitialValues<K = any, V = any> =
 
 // just extend Map? See also https://gist.github.com/nestharus/13b4d74f2ef4a2f4357dbd3fc23c1e54
 // But: https://github.com/mobxjs/mobx/issues/1556
-// TODO: introduce IObservableMap, and make ObservableMap constructor non public
 export class ObservableMap<K = any, V = any>
     implements Map<K, V>, IInterceptable<IMapWillChange<K, V>>, IListenable {
     [$mobx] = ObservableMapMarker
@@ -117,7 +116,7 @@ export class ObservableMap<K = any, V = any>
 
         let entry = this.hasMap_.get(key)
         if (!entry) {
-            // todo: replace with atom (breaking change)
+            // TODO: replace with atom (breaking change)
             const newEntry = (entry = new ObservableValue(
                 this.has_(key),
                 referenceEnhancer,
@@ -429,14 +428,12 @@ export class ObservableMap<K = any, V = any>
      * See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/observe
      * for callback details
      */
-    // TODO: kill
     observe_(listener: (changes: IMapDidChange<K, V>) => void, fireImmediately?: boolean): Lambda {
         if (__DEV__ && fireImmediately === true)
             die("`observe` doesn't support fireImmediately=true in combination with maps.")
         return registerListener(this, listener)
     }
 
-    // TODO: kill
     intercept_(handler: IInterceptor<IMapWillChange<K, V>>): Lambda {
         return registerInterceptor(this, handler)
     }

src/types/observableobject.ts

@@ -38,11 +38,6 @@ import {
     hasProp
 } from "../internal"
 
-// TODO: kill
-export interface IObservableObject {
-    "observable-object": IObservableObject
-}
-
 export type IObjectDidChange<T = any> =
     | {
           name: PropertyKey
@@ -250,33 +245,6 @@ export class ObservableObjectAdministration
         }
     }
 
-    // TODO: is this still needed?
-    illegalAccess_(owner, propName) {
-        /**
-         * This happens if a property is accessed through the prototype chain, but the property was
-         * declared directly as own property on the prototype.
-         *
-         * E.g.:
-         * class A {
-         * }
-         * extendObservable(A.prototype, { x: 1 })
-         *
-         * classB extens A {
-         * }
-         * console.log(new B().x)
-         *
-         * It is unclear whether the property should be considered 'static' or inherited.
-         * Either use `console.log(A.x)`
-         * or: decorate(A, { x: observable })
-         *
-         * When using decorate, the property will always be redeclared as own property on the actual instance
-         */
-        __DEV__ &&
-            console.warn(
-                `Property '${propName}' of '${owner}' was accessed through the prototype chain. Use 'decorate' instead to declare the prop or access it statically through it's owner`
-            )
-    }
-
     /**
      * Observes this object. Triggers for the events 'add', 'update' and 'delete'.
      * See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/observe
@@ -370,38 +338,28 @@ export function generateObservablePropConfig(propName) {
     )
 }
 
-function getAdministrationForComputedPropOwner(owner: any): ObservableObjectAdministration {
-    // TODO: what again does this function?
-    const adm = owner[$mobx]
-    if (!adm) {
-        return owner[$mobx]
-    }
-    return adm
-}
-
 export function generateComputedPropConfig(propName) {
     return (
         computedPropertyConfigs[propName] ||
         (computedPropertyConfigs[propName] = {
             configurable: true,
             enumerable: false,
             get() {
-                return getAdministrationForComputedPropOwner(this).read_(propName)
+                return this[$mobx].read_(propName)
             },
             set(v) {
-                getAdministrationForComputedPropOwner(this).write_(propName, v)
+                this[$mobx].write_(propName, v)
             }
         })
     )
 }
 
-// TODO: extract constant for "ObservableObject ?
 const isObservableObjectAdministration = createInstanceofPredicate(
     "ObservableObjectAdministration",
     ObservableObjectAdministration
 )
 
-export function isObservableObject(thing: any): thing is IObservableObject {
+export function isObservableObject(thing: any): boolean {
     if (isObject(thing)) {
         return isObservableObjectAdministration((thing as any)[$mobx])
     }

src/types/observableset.ts

@@ -58,7 +58,6 @@ export type ISetWillChange<T = any> =
           newValue: T
       }
 
-// TODO: introduce IObservableSet, and make ObservableSet constructor non public
 export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillChange>, IListenable {
     [$mobx] = ObservableSetMarker
     private data_: Set<any> = new Set()
@@ -118,7 +117,7 @@ export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillCh
                 newValue: value
             })
             if (!change) return this
-            // TODO: ideally, value = change.value would be done here, so that values can be
+            // ideally, value = change.value would be done here, so that values can be
             // changed by interceptor. Same applies for other Set and Map api's.
         }
         if (!this.has(value)) {
@@ -234,16 +233,13 @@ export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillCh
 
         return this
     }
-
-    // TODO: kill
     observe_(listener: (changes: ISetDidChange<T>) => void, fireImmediately?: boolean): Lambda {
-        // TODO 'fireImmediately' can be true?
+        // ... 'fireImmediately' could also be true?
         if (__DEV__ && fireImmediately === true)
             die("`observe` doesn't support fireImmediately=true in combination with sets.")
         return registerListener(this, listener)
     }
 
-    // TODO: kill
     intercept_(handler: IInterceptor<ISetWillChange<T>>): Lambda {
         return registerInterceptor(this, handler)
     }

src/types/observablevalue.ts

@@ -126,16 +126,11 @@ export class ObservableValue<T> extends Atom
         return this.dehanceValue(this.value_)
     }
 
-    // TODO: kill?
-    public intercept_(handler: IInterceptor<IValueWillChange<T>>): Lambda {
+    intercept_(handler: IInterceptor<IValueWillChange<T>>): Lambda {
         return registerInterceptor(this, handler)
     }
 
-    // TODO: kill?
-    public observe_(
-        listener: (change: IValueDidChange<T>) => void,
-        fireImmediately?: boolean
-    ): Lambda {
+    observe_(listener: (change: IValueDidChange<T>) => void, fireImmediately?: boolean): Lambda {
         if (fireImmediately)
             listener({
                 object: this,

src/utils/utils.ts

@@ -149,7 +149,6 @@ const hasGetOwnPropertySymbols = typeof Object.getOwnPropertySymbols !== "undefi
  * Returns the following: own keys, prototype keys & own symbol keys, if they are enumerable.
  */
 export function getPlainObjectKeys(object) {
-    // TODO: use Reflect.ownKeys!
     const keys = Object.keys(object)
     // Not supported in IE, so there are not going to be symbol props anyway...
     if (!hasGetOwnPropertySymbols) return keys

test/v4/base/array.js

@@ -70,15 +70,16 @@ test("test1", function() {
     expect(a.slice()).toEqual([1, 2])
 
     expect(a.reverse()).toEqual([2, 1])
-    expect(a.slice()).toEqual([1, 2])
+    expect(a).toEqual([2, 1])
+    expect(a.slice()).toEqual([2, 1])
 
     a.unshift(3)
     expect(a.sort()).toEqual([1, 2, 3])
-    expect(a.slice()).toEqual([3, 1, 2])
+    expect(a.slice()).toEqual([1, 2, 3])
 
-    expect(JSON.stringify(a)).toBe("[3,1,2]")
+    expect(JSON.stringify(a)).toBe("[1,2,3]")
 
-    expect(a[1]).toBe(1)
+    expect(a[1]).toBe(2)
     a[2] = 4
     expect(a[2]).toBe(4)