Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#10005 closed task (invalid)

WidgetSet.map() inconsistent return value

Reported by: Les Owned by:
Priority: low Milestone: tbd
Component: Dijit Version: 1.3.2
Keywords: WidgetSet Cc:
Blocked By: Blocking:

Description

WidgetSet?.map() should return another WidgetSet?, not an array. NodeList?, for example, maps to a NodeList?, not an array. If I need an array, I can use toArray(). I think WidgetSet?.map() is a new feature for 1.4, so there should be no back compatibility issue.

Change History (9)

comment:1 Changed 10 years ago by dante

if we change this we need to do it now and quickly (and update the docs I added to campus) before we cut 1.4

comment:2 Changed 10 years ago by Adam Peller

Priority: normalhigh

comment:3 Changed 10 years ago by Les

...or map to a NodeList?. WidgetSet?.add() and WidgetSet?.byClass() won't make sense if the WidgetSet? does contain widgets. Also, WidgetSet?.toArray() could be replaced by WidgetSet?.toNodeList().

comment:4 Changed 10 years ago by Les

Correction:

WidgetSet?.add() and WidgetSet?.byClass() won't make sense if the WidgetSet? does NOT contain widgets.

comment:5 Changed 10 years ago by bill

Priority: highlow
Resolution: invalid
Status: newclosed
Type: defectenhancement

Sorry, but it's operating according to spec already, returning an array as specified.

As you said WidgetSet's add(), byClass(), etc. functions wouldn't make sense on a return value of (for example) [1,2,3] or ["a", "b", "c"], and also, WidgetSet is a hash (name --> value map) whereas map() return an Array (ordered list of items). They are different data structures. Some users surely have code like dojo.forEach(myWidgetSet.map(...), ... that would break if we made this change.

I think what you are really asking for is something like #4122, if you'd like to work on that refactor that would be great, although would need to weigh the added convenience against the performance degradation of removing an item.

comment:6 Changed 10 years ago by Les

I wrote some code. Returning NodeList? is better b/c it's a beefed up Array, so it has e.g. forEach. This won't break any existing code b/c NodeList? is an Array subclass.

dijit.WidgetSet.prototype.map = function(func, thisObj){
	thisObj = thisObj || dojo.global;
	var ar = new dojo.NodeList(), x = 0, i;
	for(i in this._hash){
		var ret = func.call(thisObj, this._hash[i], x++, this._hash);
		ar.push(ret);
	}
	return ar; // NodeList
}

comment:7 Changed 10 years ago by bill

OK, yes, returning a beefed up array sounds more reasonable. Although, I find it odd for map() to return a NodeList since it's likely not returning a list of nodes (could be returning a list of id's, etc.).

I don't really see why this should be specific to WidgetSet.map() though. If you want such functionality why not ask for it as part of dojo.map()? If dojo.map() returned augmented arrays it would "fix" WidgetSet.map() and also anyone else using that method.

In the meantime users can simply do something like this, I think:

new NodeList(myWidgetSet.map(...))

comment:8 Changed 10 years ago by Les

Nope, the example won't work, although I wish it was this easy to convert from Array to NodeList?.

var arr = [1, 2, 3];
new dojo.NodeList(arr).length // returns 1

I'like when dojo or dijit methods return NodeList? b/c I can use list.forEach... and not dojo.forEach(list...

NodeLists? are really nice, but the name is misleading. It's easy to assume that a NodeList? must contains nodes, but it doesn't. NodeList? is just a decorated Array.

comment:9 Changed 10 years ago by bill

Type: enhancementtask

seems like these aren't really enhancements from the users' point of view, labeling as tasks instead

Note: See TracTickets for help on using tickets.