Move Observable, etc. in liaison and delite/Stateful to decor.

[asudoh]

For ibm-js/delite#178.

[wkeese]

I have some line-item comments I'll add tomorrow, but I want to discuss the API to Observable itself first.

The API doc and test file use Observable as a base class, i.e. calling new Observable(). Yet Stateful does not extend Observable. Rather, you turn Stateful instances into observable objects by calling Observable.call(this).

Relatedly, I think people will be confused that Observable is apparently a base class, yet it has lots of static methods like observe(), getNotifier(), deliverChangeRecords().

So, what I would suggest is one of two paths:

(1) Just make Observable a base class, and give it (non-static) methods like getNotifier() used without any arguments i.e. instead of:

var myClass = new Observable();
Observable.getNotifier(myClass)

users would do:

var myClass = new Observable();
myClass.getNotifier()

This is a nice API to use but it won't work if you need to retrofit existing objects to be observable.

(2) Match the harmony spec more closely, by removing the Observable.call(this) and new Observable() API's, and instead just have
Observable.observe(myClass), Observable.getNotifier(myClass), Observable.deliverChangeRecords(myClass).

The one big deviation from the spec though is having to use .set() to update properties of observed objects, rather than just setting them directly, so while technically true, it seems misleading to say that you can observe any object.

[asudoh]

liaison/Observable as well as the new decor/Observable took the route close to 2) above, which is aligning to standard where possible and makes sense. As @wkeese seems to have noticed, that’s one key reason why most of the key Observable methods are static. Another reason of static methods is enumeration; You may notice that the only non-static method, .set(), is non-enumerable so that for … in loop as well as future ES6 Object.assign() (“mixin" in our jargon) work as users normally expect. But something like .set() is really hard to use if it’s a static method.

Observable’s key deviations stem from the nature that Observable is a “container object” (ref), whereas Object.observe() API works with a plain object. It means that Observable needs a constructor so that application can easily create a data model. Observable is not only for Stateful; Its another key purpose is for application’s data model where application often creates instances directly with its constructor. Observable has a non-enumerable .set() method for automatic change record emission.

In case it’s not been made clear enough; Observable.observe() can observe any object only if Object.observe() is available natively in browser. Otherwise only objects that emit change records via Observable’s notifier API can be observed.

The confusion @wkeese felt may (in part) stem from the fact that Stateful does not really inherit Observable. This is from my impression that DCL does not care about property descriptors which Observable heavily relies on, and it may be broken by creating inheritances by DCL. But if @uhop or somebody can ensure that it’s not true or provide a roadmap in DCL to support property descriptor we may reconsider here.

[wkeese]

Actually, I'm not confused. What I said was that the API is confusing. So:

1. Apparently, we need for(var key in obj) to just iterate the user defined properties of the object, and skip functions like set(). This is strange because none of the other classes in ibm-js or dojo (for example, Stateful and Widget) do that . But maybe there is a special reason that Observable needs to.
2. You could easily make Observable object oriented (my suggestion (1)) by adding non-iterable methods to the decorated object. You are already adding set(), so you could easily add getNotifier() etc. too. Alternately set() could be a static method, Observable.set(myObj, "name", val). It's inconvenient to use, but the same is true for all of Observable's static methods. So it's really just a question of what we want to do.
3. It's true that DCL doesn't have any provisions for dealing with property accessors; that's why Stateful has the introspect code to convert "normal" methods like _setFooAttr().

[asudoh]

In case I was not clear enough; Given the main purpose of Observable is for application's data model, instead of just for Stateful#observe(), making .set() enumerable means that forcing application code to skip it in enumeration code. In other words, it's not just our code having to do so.

Also to elaborate on why set() is non-static while other methods are static; Something like .set() is really hard to use if it’s a static method, but other than that, it'd make sense to keep other methods static so that people knowing Object.observe() can use Observable.observe() in the same way they know. (Less escalator of acquired knowledge)

[wkeese]

Thanks for your updates. Can you push your updates as separate commits, rather than using push --force, and overwriting your original commit? What you are doing is tantamount to erasing all my (and cjolif's) comments. Turns out the comments are still saved in asudoh@88b88b2#commitcomment-6855378, but they are lost from the PR, and the only way to get to them is if you know the original commit hash.

[asudoh]

OK from now on I’ll squash after review completes.

[wkeese]

I see you're a big fan of making variables super-private, i.e. private to a specific function definition, by using IEF's. We don't generally do that in ibm-js, so I'm wondering what @cjolif etc. think about this. I see the reason you do it, but personally I don't think it's worth having the extra code and indentation.

I'm seeing six IEF closures in your code that I think should be simplified:

(1) var observableMarker = "_observable"

You have a closure to define an observableMarker variable, which you then use in three places:

(function () {
    var observableMarker = "_observable";

    Observable = function (o) {
        if (!this[observableMarker]) { ...}
        ...
    };

    Observable.test = function (o) {
        return o && o[observableMarker];
    };
})();

If you just hardcode _observable (o._observable rather than o[observableMarker]) it will save having that IEF.

(2) For defining set(), you use a closure to hide the areSameValues function so that it can only be used by the set() method:

defineProperty(Observable.prototype, "set", { // Make set() not enumerable
    value: (function () {
        var areSameValues = has("object-is-api") ? Object.is : function (lhs, rhs) { ...
        return function (name, value) {... };
    })(),

It could be written more simply as:

var areSameValues = has("object-is-api") ? Object.is : function (lhs, rhs) { ...
defineProperty(Observable.prototype, "set", { // Make set() not enumerable
    value: function (name, value) {... };

(3) In the main part of the code you have three level of IEF's.

(function () {
    var seq = 0, ...;
    Observable.getNotifier = (function () {
        function Notifier(target) { ... }
        Notifier.prototype = {
            notify: (function () {
                function enqueue(changeRecord) { ... }
                return function (changeRecord) { ... };
            })(),
            ...
        };
        return function (observable) { ... };
    })();

    Observable.observe = (function () {
        var DEFAULT_ACCEPT_CHANGETYPES = ...; // Observable#set() only supports the first two
        return function (observable, callback, accept) { ... };
    })();
    ...
})();

For the notifier method with the function helper, we would typically write that as:

notify: function(changeRecord){
    function enqueue(changeRecord) { ... }
    // main code
}

instead of:

notify: (function () {
    function enqueue(changeRecord) { ... }
    return function (changeRecord) { 
        // main code
    };
})(),

As for the other levels of nesting, they are just to hide variables within a certain section of the file, which I can see the merit of, but it doesn't seem very important, so my preference is to save a few bytes by just using comments in the code like:

///////////////////////////////////
// Notifier section
///////////////////////////////////

or something like that.

[asudoh]

@cjolif I think it's your call here?

[cjolif]

Sorry, you go too fast for me guys to keep up ;)

I guess I'm more or less in agreement with @wkeese here. In theory I do like the idea of scoping variables to the more limited scope and I understand why you are doing that, however, adding this piece of code is already none negligibly adding to the size of decore/delite and we also need to keep in mind we don't want to be "too big", so any effort we can do to reduce the code size while still keeping it correct is important I think.

Also there is the consistency issue as indeed other part of the code are not heavily relying on those techniques and mixed style does not facilitated reading and maintaining the code.

In summary I guess yes it would make sense to have some level of simplification here.

[asudoh]

OK made the change: asudoh@6ad17b3

[wkeese]

This is looking good to me now. The only outstanding thing I see is the duplicated areSameValues function (in Observable.js and Stateful.js). It's only one line, but since Observable is a (partial) shim for the methods in ES6's Object, it probably makes sense to export areSameValues as Observable.is().

[asudoh]

Thanks @wkeese for the thorough review. Introduced Observable.is() in asudoh@a807d68. If nobody objects, I'll squash the commits soon. (Update: Done: asudoh@77cce14)