Opened 7 years ago

Closed 6 years ago

#16097 closed defect (fixed)

Tree: set("path") promise doesn't return nodes array

Reported by: Wouter Hager Owned by: bill
Priority: low Milestone: 1.9
Component: Dijit Version: 1.8.0
Keywords: Cc:
Blocked By: Blocking:

Description

Setting the path/paths attribute on a dijit/Tree should return a promise, that returns an array containing the nodes and a success boolean on resolve. It did so in dojo<1.8, but not anymore.

Change History (10)

comment:1 Changed 7 years ago by bill

Component: GeneralDijit
Milestone: tbdfuture
Owner: set to bill
Summary: dijit.Tree: set("path") promise doesn't return nodes arrayTree: set("path") promise doesn't return nodes array

Hmm, well it's true the behavior did change. In 1.7, in test_Tree.html:

> dijit.byId("mytree").set("path", ["continentRoot", "SA", "BR"]).then(function(val){console.log("resolved promise:" + val);})
resolved promise:true,[Widget dijit._TreeNode, dijit__TreeNode_45]

In trunk, in test_Tree.html:

> dijit.byId("mytree").set("path", ["continentRoot", "SA", "BR"]).then(function(val){console.log("resolved promise: " + val);})
Open of node undefined
Open of node South America
resolved promise: undefined

The 1.7 Promise value though is very strange: an array where the first value is a boolean and the second value is a TreeNode, plus the fact that the number of elements in the specified path (3) doesn't match the number of elements in the Promise value (2).

In other words, the value of the Promise returned from set("path") were never documented or intended as part of the API.

For set("paths", ...), the Promise value in 1.7 is even stranger. It's a concatenation (ie, flattening) of the arrays that would be returned by multiple set("path", ...) calls:

dijit.byId("mytree").set("paths", [["continentRoot", "SA", "BR"], ["continentRoot", "SA", "AR"]]).then(
    function(val){console.log("resolved promise: " + val);
})
resolved promise: true,[Widget dijit._TreeNode, dijit__TreeNode_45],true,[Widget dijit._TreeNode, dijit__TreeNode_46]

So,

  1. I think it's reasonable to want set("path", ...) and set("paths", ...) to return the same thing as get("path") and get("paths"), but I'm not interested of maintaining that strange 1.7 behavior.
  2. I'm sorry for the change of behavior, but no user code should be depending on the Promise value currently (or previously) returned by set("path", ...)
Version 0, edited 7 years ago by bill (next)

comment:2 Changed 7 years ago by Wouter Hager

I thought this was documented behavior, but perhaps I'm wrong. I can't seem to find it anymore anyway. For my purposes it would be really useful to see at least if the path did actually select anything.

comment:3 Changed 7 years ago by bill

Oh, well that should be easy to detect, by whether the promise resolves or gets an error:

myTree.set("path", ...).then(
   function(){ console.log("path set successfully to", myTree.get("path")); },
   function(){ console.log("error setting path"); }
);

comment:4 Changed 7 years ago by Wouter Hager

This is the behavior I expected, but my tests do not call errBack. However, get("path") returns an empty array, so this is the only option I seem to have.


dijit.registry.byId("mytree").set("path",["continentRoot","AF","EG"]).then(function(res){
  console.log(res); // returns undefined, as we know
  console.log(tree.get("path")); // returns array of items
},
function(err){
  console.error(err); //never called
});

dijit.registry.byId("mytree").set("path",["continentRoot","AF","XX"]).then(function(res){
  console.log(res); // returns undefined, as we know
  console.log(tree.get("path")); // returns empty array
},
function(err){
  console.error(err); // never called
});
Last edited 7 years ago by Wouter Hager (previous) (diff)

comment:5 Changed 7 years ago by bill

Hmm, that's strange, we do have a unit test that the promise goes to an error state:

function setInvalidPath(){
    var d = new doh.Deferred();
    myTree2.set("path", ["earth", "AF", "KE", "Narnia"]).then(
        function(){
            d.errback("Should have gotten error trying to set invalid path");
        },
        d.getTestCallback(function(e){
            doh.t(e instanceof Tree.PathError, "got PathError");
        })
    );

    return d;
}

Note sure why the unit test is working but the above case isn't. I'll take a look.

comment:6 Changed 7 years ago by Wouter Hager

I think that's because doh is using legacy deferred.

comment:7 Changed 7 years ago by bill

my tests do not call errBack

Actually, in trunk (but not 1.8) your test case also calls the errback function, where err.message is "Could not expand path at XX". Fixed in [29568].

Last edited 6 years ago by bill (previous) (diff)

comment:8 Changed 7 years ago by bill

Priority: undecidedlow

comment:9 Changed 6 years ago by bill

Milestone: future1.9
Status: newassigned

comment:10 Changed 6 years ago by bill

Resolution: fixed
Status: assignedclosed

In [31047]:

Add return type for Tree.set("paths") and Tree.set("path"). Fixes #16097 !strict. (The main part of the ticket about throwing an error for invalid paths was fixed earlier.)

Note: See TracTickets for help on using tickets.