Accessor class for Dojo

[kitsonk]

Dojo should have a base class that provides access to accessors. Currently dijit/_WidgetBase has the concept of auto-magic accessors, but they do not exist outside of Dijit. Also, since ES5 defineProperty is not fully shimable an alternative approach should be taken to allowing a consistent way of generating accessors that also builds upon dojo/Evented and dojo/Stateful classes.

I have a patch (attached) that provides such a mechanism, called dojo/Attributed. Creating a class with auto-magic accessors would work like this:

define(["dojo/_base/declare", "dojo/Attributed"], function(declare, Attributed){
  return declare([Attributed], {
    foo: "",
    _get_foo: function(){
      return this.foo;
    }),
    _set_foo: function(value){
      this.foo = value;
    });
  });
});

And end developer usage would work like this (assuming the above class' module was package/AttributedClass):

require("package/AttributedClass", function(AttributedClass){
  var myAttributedClass = new AttributedClass();
  myAttributedClass.on("changed", function(e){
    console.log("Attribute Changed:", e.attrName, e.prevValue, e.newValue);
  });
  myAttributedClass.on("changed-foo", function(e){
    console.log("foo changed:", e.prevValue, e.newValue);
  });
  myAttributedClass.watch("foo", function(name, oldValue, value){
    console.log("Watched:", name, oldValue, value);
  });

  myAttributedClass.set("foo", "bar");
  console.log(myAttributedClass.get("foo");
});

The class does support "legacy" Dijit accessors of _setXxxAttr and _getXxxAttr and supports global accessors of _getter and _setter.

[kitsonk]

dojo/Attributed including DOH Unit Tests

[kitsonk]

Based on feedback from the developer forum, I am attaching a patch the rolls most of the functionality into Stateful.

The class no longer supports legacy accessors, no longer emits events but handles a deferred return and won't call the watch until it is resolved.

[kitsonk]

Enhances dojo/Stateful including D.O.H. unit tests

[markwubben]

In our app data-binding works via Stateful instances, but the template layer is aware of promises. E.g. we'd use markup like this:

{{%when results}}
  <p>There are {{results|length}} results.</p>
{{%until}}
  <p>Fetching results…</p>
{{/%}}

Not having the watch callbacks trigger when a promise is set would break this behavior. Promises are values too! Similarly, setting an attribute to a promise *does* change the original value, but no watch callback is triggered which may confuse internal state. And what happens when the promise is rejected? The watch callbacks are never triggered.

I do see value (no pun intended) in waiting until the promise resolves, but perhaps it should be opt-in or opt-out?

[kitsonk]

To clarify, setting a property to a promise will not cause anything odd to occur. The behaviour only kicks in when the setter returns a promise, thereby indicating that it has not yet set the value. If the setter sets the property to a promise, then the getter would read it and return it. The challenge from Ben was that the setters should return a consistent value, which is in Stateful is this.

Does that make more sense?

[markwubben]

Aha, missed that!

Don't think the set_foo methods need to return this though, as they're wrapped by a set()?

[kitsonk]

Yes.

What happens is that set_foo can either a) set the attribute an if the return value is anything other than a promise, it is ignored or b) it can return a promise, which will cause that set() to wait until the promise is fulfilled before calling the watches (via using when). So it is up to the set_foo on when the watches are called.

If the attribute/property is a promise, set_foo should just set the value and either return nothing or return a non-promise value.

[kzyp]

I think this looks great. Is it problematic that it doesn't support legacy accessors? Will that prevent it from being used in Dijit? Anyway, I think it is great idea either way, let's do it. Do you want me to commit it? I think you are a committer now, right Kelly?

[kitsonk]

Yes I am.

I will be on the meeting this week, and I know Bill and I have chatted about this and how it would fit into _WidgetBase and the legacy accessors. So I will have another chat and then move forward.

[kitsonk]

In [28505]:

Add custom accessors to dojo/Stateful, fixes #15187 !strict

[edchat]

Hi Kitson, this update has broken a dojox.mvc test. It looks like the change to postscript is causing the problem, but there may be more to it than that. postscript used to call mixin like this:

postscript: function(mixin){

if(mixin){

lang.mixin(this, mixin);

}

}

And if I change it back to this the test works. You can run this test to see the problem: dojox/mvc/tests/doh_mvc_loan-stateful.html

Thanks, Ed

[kitsonk]

Ok, I will take a look as to why it is breaking, but if we just mixin than any custom setters won't get called. I suspect it is because whatever is being mixed in isn't present in the original class.

[edchat]

It looks like I can fix it in dojox/mvc/Stateful by adding this to the set function:

if(typeof name === "object"){

for(var x in name){

this.set(x, name[x]);

} return this;

}

[kitsonk]

That would be a better way, since dojo/Stateful accepted an object anyways (as well as dijit/_WidgetBase, etc.).

Are you comfortable fixing it dojox/mvc/Stateful then?

[edchat]

Yes I will update dojox/mvc/Stateful, sorry I reopened this one...

[neonstalwart]

r28505 also reverted the hasOwnProperty check added in r27488 that allowed a Stateful to be passed as the value to another Stateful when setting. i haven't checked if it still works or not - was the hasOwnProperty check intentionally removed and is this use case still supported?

[kitsonk]

No, it wasn't intentional, but the removal of it didn't cause the unit tests to fail.

I will revert it anyways, although it seems to be non-needed code.

[kitsonk]

In [28519]:

Revert unintended removal of hasOwnProperty, refs #15187

[bill]

Replying to edchat:

Yes I will update dojox/mvc/Stateful, sorry I reopened this one...

I filed #15309 to update dojox/mvc/Stateful.

[neonstalwart]

Another oversight... Previously you could cleanly serialize a Stateful to JSON without any of the "internals" of the class bleeding into the serialization. It looks like _attrPairNames will now pollute the serialization of Stateful. How about putting the cache on the _getAttrNames function? I haven't actually checked this problem occurs so let me know if I'm wrong.

Also, if I'm right about this then I guess there must not be tests for this (or the previous use case I mentioned). We should add some tests since these are both very useful qualities and worth making sure they don't regress. If you don't beat me to it I'll see if I can add some tests before the end of the coming week.

[kitsonk]

In [28522]:

Add serialization test case for dojo/Stateful, refs #15187

[kitsonk]

I have checked in a test case for serialization, but this patch doesn't appear to regress the functionality. I suspect it is because it is part of the prototype and because it isn't written directly to the instance, it doesn't appear when the object is serialized.

There is a test for the previous case (part of r27488), but like I mentioned, it didn't cause the test case to fail, which is why I didn't notice I had accidentally removed it. So I don't know what sort of test case would actually trigger that code, because assigning one dojo/Stateful to another seems to work perfectly fine.

[neonstalwart]

kitson, thanks for checking the serialization and adding a test - i neglected to take into account that only own properties will be serialized to JSON.

however, i did find an issue with setting one Stateful via another. it is an issue that has existed since before your changes so i opened a new ticket for it at #15315.

[kitsonk]

Ok, I will close this ticket now, since everything related to it is in progress under other tickets.

[kitsonk]

In [29089]:

Removes incorrect comment, refs #15187

[csnover]

In [30650]:

Fix incorrect non-portable reference to dojo package. Refs #15187. Backport to 1.8

[csnover]

[30649] refs this but I messed up and put the bad commit ID into the commit message instead and apparently nothing actually checks that. I like git commit IDs even more now.