Improved observable api

[mweststrate]

Improved observable api

[urugator]

It shouldn't throw, but simply assign boxed computed:

const o = {}
mobx.extendObservable(o, {
  boxedComputed: mobx.computed(() => "a"), 
});
console.log(o.boxedComputed.get());

[mweststrate]

This would create an observable references to a computed object, it is barely imaginable that that is what the user would intend, so throwing and telling how to properly do it is better imho.

If he really needs a boxed computed, he can just do o.boxedComputed = mobx.computed(). Or in the weird case hey needs an observable ref to it, extending it with null first, and then assigning it would work fine.

[urugator]

If you try to re-decorate prop (via decorate/extendObservable) it throws:
@observable can not be used on getters, use @computed instead
Because it tries to apply decorator on mobx generated getter...

[urugator]

If you mean to directly define observable on target based on decorator type, then it can't be used on prototypes ...?

[mweststrate]

The problem is that we can't work with prototypes in the same way as with objects directly. It is kinda stupid, but the iterability will be wrong, if you define a property with getter / setter on the prototype, the properties won't be interated by most tools that iterate objects, because they are not own properties of the instances. So JSON.stringify, console.log, Object.keys all will behave weird.

Hence the decorator version is much more complex then the non-decorator implementation. Basically; it creates a property on the prototype / object, that, upon first read, will create a new property definition on this.

In the implementation in this branch I only used the decorator based implementation to test the new api, but it is effectively twice as slow. This is also the reason decorate and extendObservable will both remain as api; extendObservable (and observable.object) can short circuit that process because they should always be used on instances.

[mweststrate]

In pseudo code, it is something like this:

function decorator(target, prop) {
    Object.defineProperty(target, prop, {
        enumerable: true, 
        configurable: true,
        get() {
            return this._x
        },
        set(v) {
            // on first write, redeclare the prop (also do on first read btw)
            Object.defineProperty(this, prop, {
                enumerable: true,
                get() {
                    return 3
                },
                set(v) {
                    this._x = v * 2
                }
            })
            this._x = v * 2
        }
    })
}

class X {
    @decorator y = 3
}

const x = new X()

console.dir(JSON.stringify(x))

I have some ideas on how that can be improved though, but not sure yet if it will work out :)

[mweststrate]

N.B, note that the above still has weird behavior;

const x = new X()
Object.getOwnPropertyNames(x) // empty set!

The reason is; before the property is read or written once, it does't exist on the instance yet. This is luckily a problem that will be fixed in the new decorators proposal.

[urugator]

Yea I think I understand that, but basically it's just a peformance problem ... if target is not prototype, then "enumberable" getters are defined directly on the object, therefore are "own" and enumerable... and if target is prototype then props are not "owned" by instance until first get ... therefore it doesn't introduce any new problems ...

So, if I understand correctly, after optimization, the decorators passed to extendObservable won't be actually called... therefore the depedency on decorators is technically redundat and for example strings/symbols could be used instead...?:

extendObservable(o, values, {
  a: "ref",
  b: "deep",
})

[mweststrate]

If you try to re-decorate prop (via decorate/extendObservable) it throws:
@observable can not be used on getters, use @computed instead
Because it tries to apply decorator on mobx generated getter...

Do you have an example test case?

[urugator]

const o = {};
mobx.extendObservable(o, {
  a: 5,  
});
mobx.extendObservable(o, {}, {
  a: mobx.observable.ref // a is getter at this point
});

I tried it in that codesandbox you provided with updated dep to alpha2

[mweststrate]

@urugator that is not supposed to work; extendObservable is only allowed to declare new properties (previously it was allowed, but it makes for way to many weird edge cases, including ones where you could easily leak observables)

[urugator]

Well ok, but can we do something about the error...?

[mweststrate]

yes, good one

[mweststrate]

correct, what matters is the 'enhancer', which get's passed to the observable value when constructing the final property Op do 1 mrt. 2018 om 21:34 schreef urugator <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In src/api/extendobservable.ts <#1365 (comment)>: > const adm = asObservableObject(target) startBatch() try { - for (let key in properties) - if (hasOwnProperty(properties, key)) { - if ((target as any) === properties && !isPropertyConfigurable(target, key)) continue // see #111, skip non-configurable or non-writable props for `observable(object)`. - const descriptor = Object.getOwnPropertyDescriptor(properties, key) - defineObservablePropertyFromDescriptor(adm, key, descriptor!, defaultEnhancer) + const decorators: any = {} + for (let key in properties) { + // TODO: using decorators is a bit inefficient, short circuit those Yea I think I understand that, but basically it's just a peformance problem ... if target is not prototype, then "enumberable" getters are defined directly on the object, therefore are "own" and enumerable... and if target is prototype then props are not "owned" by instance until first get ... therefore it doesn't introduce any new problems ... So, if I understand correctly, after optimization, the decorators passed to extendObservable won't be actually called... therefore the depedency on decorators is technically redundat and for example strings/symbols could be used instead...?: extendObservable(o, values, { a: "ref", b: "deep", }) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1365 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhMGuNQMvL7RLm68nJ1qndU_tFT6Gks5taFtMgaJpZM4SYXC0> .

[coveralls]

Coverage decreased (-1.3%) to 92.84% when pulling 390a852 on improved-observable-api into 3bde5bc on mobx4.