Simplify NodeList

[Eugene Lazutkin]

NodeList should be a shell for single-node functions in dojo._base.html. We should expose standard adapters, which will allow to apply function in the following fashion:

• forEach: the single node function applied to every node in the list, the list itself is returned for chaining.
• map: the single node function applied to every node in the list, the results are collected into a new array and returned for chaining.

For flexibility wrappers should understand how to pass additional parameters, and where to place the node parameter.

Users should be able to add their single-node functions using the same method.

Evaluate the applicability of following types of wrappers:

• filter
• every/some
• folds

[alex]

I'm confused...what's wrong with:

dojo.extend(dojo.NodeList, {
   ...
});

???

[Eugene Lazutkin]

Nothing wrong.

But:

• We may have more than one node wrapper.
• We need the automatic adapter (outlined in the description of the ticket), so the same function can be used with a single node, and NodeList.

[dante]

#7295 would allow for half of this ticket. I'm confused at the value of dojo.fn vs dojo.extend() or how it will make it easier (or automatic?) to apply single-node-functions to a list. exposing mapIn acts as that wrapper if I am understanding you correctly.

[Eugene Lazutkin]

Non-trivial methods in NodeList:

• orphan() --- removes all nodes (with optional filtering) from their respective parents while keeping them in the list.
  • Do we need to make a single node dojo.orphan() and delegate?
  • Do we really need the "simple" CSS filtering here?
• adopt()/place() --- mostly trivial group placement methods implemented using reduce-like algorithms.
  • These methods are good cases for reduce-like wrappers.
• query() --- group query using a NodeList as a list of roots.
  • The result is the concatenation of all individual query results.
  • Again this is the reduce pattern in action.
• filter() --- "simple" CSS-based filtering (used in orphan() too) of the list.
  • It is probably unfinished, because the code does not correspond the signature and the inline documentation.
• clone() --- stubbed, but commented out.
  • Should we take it out completely?
  • Should we implement it as the map-like wrapper for cloneNode()?
• addContent() --- the limited version of placing a clone of a node to all nodes in the list.
  • Always wraps everything in <span> (???).
  • Always uses the current document dojo.doc.
  • Is it really useful in its present form?
• empty() --- now it will be the trivial wrapper for dojo.empty().
• instantiate() --- instantiates a class using the widget/dojo.parser convention for every node in the list.
  • Is it useful generally?
  • It returns the same node list. Shouldn't it return the array of instantiated objects?
• at() --- creates a subset of the list by specifying all item indexes individually one by one.
  • The lack of group operations and the necessity to know the list size makes it unattractive. It can be easily replaced with the NodeList? constructor for realistic cases.

Some of the methods in the list are of limited usefulness. I guess we have to keep them now. We need to decide if we keep them in 2.0. OTOH given that all of them are undocumented officially we have a wiggle room.

[James Burke]

I would prefer to try to sync as much as we can with jquery methods if we go changing these methods. For instance, they have have a remove() that takes an optional query to filter down what gets "orphaned" in our lingo.

And I am fine with waiting for Dojo 2.0 -- I would like to start the 2.0 process sooner rather than later.

[Eugene Lazutkin]

Another thing I failed to mention: NodeList is not a class in the conventional sense and it is not used anywhere as such. The heart of the additional functionality is the private function called tnl

var tnl = function(arr){
  // decorate an array to make it look like a NodeList
  arr.constructor = dojo.NodeList;
  dojo._mixin(arr, dojo.NodeList.prototype);
  return arr;
}

As you can see it adds to the array instance additional methods and for some reason changes the constructor property (it is not used anywhere directly or indirectly).

While there is some code related to setting up NodeList as a class, it is short-circuited in the "constructor":

dojo.NodeList = function(){
  return tnl(Array.apply(null, arguments));
}

Effectively it returns an array instance decorated by tnl, NodeList's prototype is not used directly (but mixed in by tnl) and is not part of the inheritance chain (changes on NodeList.prototype are not reflected on existing instances).

To prove the point:

(new NodeList())   instanceof NodeList // false
dojo.query("body") instanceof NodeList // false

(new NodeList())   instanceof Array // true
dojo.query("body") instanceof Array // true

Summary: all business around NodeList is not actually used, using direct arrays decorated by tnl produces the same effect with less overhead, e.g., without passing NodeList as a undocumented argument in some functions to be used as a constructor instead of !Array.

tnl itself is not optimal:

1. The "constructor" change is not needed, and not explained.
2. Mixing in NodeList.prototype in every produced array is not optimal because for-in loop is used.

We can improve on above mentioned points.

[Eugene Lazutkin]

It looks like I was wrong in the comment 6 about filter() --- it does pass the function argument to the underlying array filter().

[Mike Wilson]

Replying to elazutkin:

var tnl = function(arr){

decorate an array to make it look like a NodeList?
arr.constructor = dojo.NodeList?;
dojo._mixin(arr, dojo.NodeList?.prototype);
return arr;

}
<snip>

1. The "constructor" change is not needed, and not explained.

I'd say the use of the constructor property is quite a standard pattern when subclassing (or emulating subclassing like in this case). The purpose is to have the constructor property reflect which method instantiated the object, which this code does correctly. (It might have been somewhat more self documenting if the constructor assignment had been done from within the NodeList? constructor, but that's details.)

As you have already elaborated on, NodeList? is not a subclass to Array, as Array cannot be subclassed (in any traditional way at least). Doing an emulated subclass like NodeList?, or going with plain arrays, is a matter of taste which I don't have an opinion about, but if staying with NodeList? I think the constructor assignment should also stay.

[Eugene Lazutkin]

the reworked NodeList?.js --- can be used as a drop-in replacement

[Eugene Lazutkin]

the reworked NodeList? tests --- can be used as a drop-in replacement

[Eugene Lazutkin]

Finally I worked up the long promised update for NodeList.js. Highlights and notes:

No API change! All existing tests work! Yay!

Four adapters are introduced: adaptAsForEach(), adaptAsMap(), adaptAsFilter(), and adaptWithCondition(). They adapt a single node function for an array-like operations. The position of the node argument can be specified (default: 0 --- the first argument). They are used in the implementation of dojo.NodeList, and exposed to our user so new methods can be easily adapted and added.

adaptWithCondition() allows to switch between forEach() and map() dynamically like _mapIntoDojo() does, but:

• It does not allocate and fill the unnecessary array (s --- see the current code for details) when it is not going to be used.
• It allows an arbitrary function to decide what branch will be used instead of relying on hard-coded conditions.

New generation of browsers came with native NodeList defined. They are returned from dojo.query(), and it breaks some of our code in the current version. For example, the native Array.prototype.concat() does not recognize NodeList as an array => does not inline it. Example:

var spans = dojo.query("span");  // [span1, span2]
var divs  = dojo.query("div");   // [div1, div2]
var snd   = spans.concat(divs);  // [spans, divs]
// should be: [span1, span2, div1, div2]

This problem is fixed in the new version.

New generation of browsers (e.g., Safari) do not support mutable constructor property. It is read only, and cannot be changed. It means that tnl cannot assign it. It breaks all code that rely on it (e.g, in our tests):

var x = dojo.query(...);
...
d.isArray(x)                     // false
x.constructor === dojo.NodeList  // false
x.constructor === NodeList       // true

Additionally I cleaned up some stuff:

• NodeList.orphan() does not use a dynamically compiled function anymore.
• NodeList.adopt() is expressed in terms of NodeList.place(), and does not provide the unnecessary default for the position argument.
• NodeList.query() is reworked to call concat() once per call rather than once per node.
• NodeList.filter() is simplified, shortened, generating an array by push() is eliminated.
• NodeList.addContent() is simpified, switched to dojo._toDom() facility making it more general.
• NodeList.empty() is delegated to dojo.empty() with a for-each-type adapter. The dynamically compiled function is eliminated --- it did not work anyway on IE for certain types of nodes (e.g., table-related). Now it works correctly everywhere.
• NodeList.instantiate() is simplified a little. A constant sub-expression was moved out of the loop.
• NodeList.concat() was reworked as described above.
• No changes: NodeList.map(), NodeList.forEach(), and NodeList.at().

Implemented with a special adapter following array methods: slice(), splice(). concat() is used to be in this list, but was reworked to accommodate native NodeList as described above.

Implemented with a special adapter from array.js: indexOf(), lastIndexOf(), every(), some().

Implemented with adaptWithCondition() from html.js: attr(), style(), marginBox(), and contentBox(). The latter two are new methods, and can be used to get a list of geometry information for given nodes.

Implemented with adaptAsFilter() from html.js: isDescendant(), hasAttr(), and hasClass(). These are new methods. While the same thing can be done dojo.query() these filters will be faster. If you have a different opinion, we can remove them and save 3 lines of code in the file.

Implemented with adaptAsForEach from html.js: connect(), addClass(), removeClass(), toggleClass(), empty(), and removeAttr(). The last one is the new method.

Implemented with adaptAsMap from html.js: coords().

New method was added: destroy() --- destroys all nodes from the list, returns nothing. Implemented as a for-each-type delegation to dojo.destroy().

Special methods for event names are left unchanged.

Now all existing NodeList-related tests work on all tested browsers (IE6-8, FF1.5-3.1, Safari, Opera, Chrome1-2).

TODO:

• Expand the test coverage. What we have now is not enough.
• Document additions (inline and at Dojo Campus).
• Expose name lists used to generate adapters as private variables --- they can be reused in other possible wrappers.

To bad we are late for 1.3 so I'll target it to 1.4.

[Eugene Lazutkin]

Related ticket: #7976.

[Eugene Lazutkin]

As agreed before I am reassigning this ticket to Alex for the review/criticism/improvements and general brainstorming.

[Eugene Lazutkin]

Closing #7669 is duplicate, because it is superseded by this ticket.

[Eugene Lazutkin]

Related to #8718.

[James Burke]

Some feedback on NodeList?-new.js:

1) The adaptAs* functions take an "n" node arg position function, but from what I can tell it is never used? I would say remove it to make the code simpler and easier to follow (as one of the FIXMEs says). If we come across a case where the node is not the first arg, we can do a function adapter for that case until we see a larger use case, then maybe reconsider the "n" use.

2) The latest _mapIn in the trunk's NodeList? allows a scope argument (defaults to dojo if not provided). This seems to allow mapping in functions that are outside the dojo namespace, for use in things like Pete's plugd code?

From what I can tell the adaptAs* functions in NodeList?-new.js call loopBody which then applies the functions using "this" as the this arg for the function. As I read it, the "this" in those cases will be the dojo.NodeList? instance when those functions execute.

It seems safer to allow specifying the "this" var: for instance, with dojo.style, being able to pass dojo to be used as the "this" for the function apply calls, sort of how _mapIn works today? I think it works out normally because most of our dojo.* functions reference dojo or d explicitly inside them instead of using "this" (or they are simple function operations), but something to consider. Asking Pete how it might affect plugd might shed some light on the best practice here.

3) Similarly, it would be good to see if Pete likes using the adaptAs* methods instead of _mapIn for his plugd stuff.

4)Size considerations: This NodeList? makes Base bigger over the old one. I removed all the new methods this patch adds: marginBox, contentBox, adaptAsFilter, isDescendant, hasAttr, hasClass, adaptAsFilter (since isDescendant, hasAttr and hasClass) are removed), removeAttr, destroy.

That got the gzipped Base size to 27,315 vs 27,269 (difference of 46 bytes). I think for documentation purposes and size purposes, it would be good to leave off the new methods added. Coupled with removing the "n" argument stuff, maybe the size would come in about the same, maybe a bit less.

Finally, If Alex has no objections to the code structure, and we can address the above points, it seems like this would be a drop-in replacement for the current NodeList? with no new APIs. In addition, it would give us fixes for:

• addContent
• concat
• empty

At the very least, I think we need fixes for addContent and concat before the 1.3 release. So I would be open to including this patch if we can get it sorted out before the next beta build for 1.3.

Otherwise, we can just pick up specific patches for addContent (see #8718) and concat (see #8620) for 1.3.

[Eugene Lazutkin]

Updating as agreed in IM: remove the "n" parameter, add the context parameter, remove all new functions, but keep adaptAsFilter, update addContent to detect the default document, if possible.

[Eugene Lazutkin]

Attached the updated version that uses dojo.global as a default context instead of this (the NodeList? instance itself). This change makes us consistent with array.js and dojox.lang.

[Eugene Lazutkin]

the update

[James Burke]

New test file for NodeList?-new.2.js that includes tests for addContent. Also includes modified tests for NodeList?.constructor to also test for native constructor.

[James Burke]

(In [16796]) Fixes #8282, applying Eugene's patch with expanded unit tests for NodeList?.addContent