Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#15187 closed enhancement (fixed)

Accessor class for Dojo

Reported by: kitsonk Owned by: kitsonk
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)

Attributed.patch (13.6 KB) - added by kitsonk 5 years ago.
dojo/Attributed including DOH Unit Tests
dojo_Stateful.patch (7.7 KB) - added by kitsonk 5 years ago.
Enhances dojo/Stateful including D.O.H. unit tests

Download all attachments as: .zip

Change History (28)

Changed 5 years ago by kitsonk

dojo/Attributed including DOH Unit Tests

comment:1 Changed 5 years ago by 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.

Changed 5 years ago by kitsonk

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

comment:2 Changed 5 years ago by 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?

Last edited 5 years ago by markwubben (previous) (diff)

comment:3 Changed 5 years ago by kitsonk

  • Owner set to kitsonk
  • Status changed from new to 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 5 years ago by markwubben

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 5 years ago by 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.

comment:6 Changed 5 years ago by 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?

comment:7 Changed 5 years ago by 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.

comment:8 Changed 5 years ago by kitsonk

  • Resolution set to fixed
  • Status changed from assigned to closed

In [28505]:

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

comment:9 Changed 5 years ago by kitsonk

  • Milestone changed from tbd to 1.8

comment:10 Changed 5 years ago by edchat

  • Resolution fixed deleted
  • Status changed from closed to 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 5 years ago by kitsonk

  • Status changed from reopened to 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 5 years ago by 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;

}

comment:13 Changed 5 years ago by 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?

comment:14 follow-up: Changed 5 years ago by edchat

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

comment:15 Changed 5 years ago by 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?

comment:16 Changed 5 years ago by 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.

comment:17 Changed 5 years ago by kitsonk

In [28519]:

Revert unintended removal of hasOwnProperty, refs #15187

comment:18 in reply to: ↑ 14 Changed 5 years ago by bill

Replying to edchat:

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

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

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by kitsonk

In [28522]:

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

comment:21 Changed 5 years ago by 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.

comment:22 Changed 5 years ago by 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.

comment:23 Changed 5 years ago by kitsonk

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:24 Changed 5 years ago by kitsonk

In [29089]:

Removes incorrect comment, refs #15187

comment:25 Changed 5 years ago by csnover

In [30650]:

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

comment:26 Changed 5 years ago by 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.

Note: See TracTickets for help on using tickets.