#15187 closed enhancement (fixed)
Accessor class for Dojo
Reported by: | Kitson Kelly | Owned by: | Kitson Kelly |
---|---|---|---|
Priority: | undecided | Milestone: | 1.8 |
Component: | Core | Version: | 1.7.2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
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
.
Attachments (2)
Change History (28)
Changed 9 years ago by
Attachment: | Attributed.patch added |
---|
comment:1 Changed 9 years ago by
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.
Changed 9 years ago by
Attachment: | dojo_Stateful.patch added |
---|
Enhances dojo/Stateful including D.O.H. unit tests
comment:2 Changed 9 years ago by
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?
comment:3 Changed 9 years ago by
Owner: | set to Kitson Kelly |
---|---|
Status: | new → assigned |
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?
comment:4 Changed 9 years ago by
Aha, missed that!
Don't think the set_foo
methods need to return this
though, as they're wrapped by a set()
?
comment:5 Changed 9 years ago by
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.
comment:6 Changed 9 years ago by
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?
comment:7 Changed 9 years ago by
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.
comment:9 Changed 9 years ago by
Milestone: | tbd → 1.8 |
---|
comment:10 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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
comment:11 Changed 9 years ago by
Status: | reopened → assigned |
---|
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.
comment:12 Changed 9 years ago by
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;
}
comment:13 Changed 9 years ago by
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?
comment:14 follow-up: 18 Changed 9 years ago by
Yes I will update dojox/mvc/Stateful, sorry I reopened this one...
comment:15 Changed 9 years ago by
comment:16 Changed 9 years ago by
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.
comment:18 Changed 9 years ago by
comment:19 Changed 9 years ago by
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.
comment:21 Changed 9 years ago by
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.
comment:22 Changed 9 years ago by
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.
comment:23 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Ok, I will close this ticket now, since everything related to it is in progress under other tickets.
comment:26 Changed 8 years ago by
[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.
dojo/Attributed including DOH Unit Tests