#8282 closed enhancement (fixed)
Simplify NodeList
Reported by: | Eugene Lazutkin | Owned by: | alex |
---|---|---|---|
Priority: | high | Milestone: | 1.3 |
Component: | Core | Version: | 1.2.3 |
Keywords: | Cc: | James Burke, dante | |
Blocked By: | Blocking: |
Description (last modified by )
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
Attachments (4)
Change History (24)
comment:1 Changed 12 years ago by
Component: | General → Core |
---|---|
Milestone: | tbd → future |
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
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.
comment:4 Changed 12 years ago by
#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.
comment:5 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Summary: | Simplify a way to do dojo.query() plugins → Simplify NodeList |
comment:6 Changed 12 years ago by
Status: | new → assigned |
---|
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?
- Do we need to make a single node
- 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.
comment:7 Changed 12 years ago by
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.
comment:8 follow-up: 10 Changed 12 years ago by
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:
- The "constructor" change is not needed, and not explained.
- Mixing in
NodeList.prototype
in every produced array is not optimal becausefor-in
loop is used.
We can improve on above mentioned points.
comment:9 Changed 12 years ago by
It looks like I was wrong in the comment 6 about filter() --- it does pass the function argument to the underlying array filter().
comment:10 Changed 12 years ago by
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>
- 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.
Changed 12 years ago by
Attachment: | NodeList-new.js added |
---|
the reworked NodeList?.js --- can be used as a drop-in replacement
Changed 12 years ago by
Attachment: | NodeList-new.html added |
---|
the reworked NodeList? tests --- can be used as a drop-in replacement
comment:11 Changed 12 years ago by
Cc: | dante added |
---|---|
Milestone: | future → 1.4 |
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 ofNodeList.place()
, and does not provide the unnecessary default for theposition
argument.NodeList.query()
is reworked to callconcat()
once per call rather than once per node.NodeList.filter()
is simplified, shortened, generating an array bypush()
is eliminated.NodeList.addContent()
is simpified, switched todojo._toDom()
facility making it more general.NodeList.empty()
is delegated todojo.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()
, andNodeList.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.
comment:13 Changed 12 years ago by
Owner: | changed from Eugene Lazutkin to alex |
---|---|
Status: | assigned → new |
As agreed before I am reassigning this ticket to Alex for the review/criticism/improvements and general brainstorming.
comment:14 Changed 12 years ago by
Closing #7669 is duplicate, because it is superseded by this ticket.
comment:16 Changed 12 years ago by
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.
comment:17 Changed 12 years ago by
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.
comment:18 Changed 12 years ago by
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
.
Changed 12 years ago by
Attachment: | NodeList-new.2.html added |
---|
comment:19 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:20 Changed 12 years ago by
Milestone: | 1.4 → 1.3 |
---|
I'm confused...what's wrong with:
???