Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider support for ES2015 iterator protocol for NodeIterator #147

Open
Ginden opened this issue Jan 12, 2016 · 12 comments
Open

Consider support for ES2015 iterator protocol for NodeIterator #147

Ginden opened this issue Jan 12, 2016 · 12 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@Ginden
Copy link

Ginden commented Jan 12, 2016

ES2015 introduces support for iteration protocols. Iterators are objects with .next method which return object {value: value, done: true/false}.
Iterables are objects with Symbol.iterator method that returns iterator.

Making NodeIterator an iterator would be non-breaking change that will allow simple iteration of object using for-of loop or converting results to static array by spread operator.

Simplest implementation would be:

NodeIterator.prototype.next = function() {
     const value = this.getNextNode();
     return {
          value,
          done: !!val
     };
};
NodeIterator.prototype[Symbol.iterator] = function() {
   return this;
}

I think real specification should include cloning "initial state" of NodeIterator to allow multiple iterations over the same NodeIterator.

Similar logic can be applied to XPathResult objects that expose iterateNext method.

Related Firefox bug.

@annevk
Copy link
Member

annevk commented Jan 12, 2016

@bzbarsky doing this would require quite some IDL support I think.

Do we really care enough about these objects to continue to maintain them and add new features?

@marcoscaceres
Copy link
Member

I ran into this too. As there is no way to select Text Nodes with selectors, using Node Iterator is still something we need to fallback to. It mean having to create an node iterator, for which I then needed an ES iterator.

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Aug 23, 2016
@annevk annevk added the addition/proposal New features or enhancements label Apr 11, 2019
@justinfagnani
Copy link

I would love to see TreeWalker be iterable as well.

@marcoscaceres
Copy link
Member

Agree also about TreeWalker... Having to use a generator is not ideal. For example:

https://github.com/w3c/respec/blob/9859c7bb2e501a15d9836c509e75069680779b43/src/core/utils.js#L757-L773

@keithamus
Copy link

@annevk would adding iterable<Node>; to the NodeIterator idl suffice?

@annevk
Copy link
Member

annevk commented Sep 20, 2023

You would also have to define the behavior. But if we wanted people to use these APIs we should also add constructors and modernize them a bit. It's just not clear to me they have the use that warrants such an investment. If you compare [javascript] NodeIterator and [javascript] querySelector on Stack Overflow the difference is quite stark. (There's also other drawbacks such as them executing script while traversing, making for expensive boundary hopping.)

@keithamus
Copy link

To me it's one of those areas where it's not used often but when it's needed there are no alternatives (same for TreeWalker) - that I know of.

I'm happy to put the work in to define what iterable<Node>; on both would do, along with browser patches. I'm also happy to modernise both APIs to become constructors, and maybe while we're at it clean up the whatToShow/filter params. Alternatively if you think there's a better direction to go in, for example defining a new mechanism for iterating over nodes, I'm happy to pursue that.

@domenic
Copy link
Member

domenic commented Sep 20, 2023

I mean the alternative is to just write the iteration code yourself, right? That'll likely be faster due to the lack of boundary-crossing, and it'll be easier to read and understand without someone having to go look up this esoteric API. I think the first step here would be demonstrating why it's so bad to write loops in JavaScript.

@keithamus
Copy link

As in manually looping over childNodes?

@annevk
Copy link
Member

annevk commented Sep 20, 2023

Yeah, creating your own tree walker. You could use childNodes, but also firstChild and nextSibling, etc.

@keithamus
Copy link

Do you think it is worth documenting this in MDN - discouraging use of TreeWalker/NodeIerator in favour of iterating over these properties?

@iansan5653
Copy link

iansan5653 commented Sep 27, 2023

I agree, these APIs definitely definitely aren't perfect. There is confusing overlap between whatToShow and filter, the difference between NodeFilter.FILTER_REJECT and NodeFilter.FILTER_SKIP is definitely not intuitive, and the difference between TreeWalker and NodeIterator is surprisingly subtle. And, of course, they don't support the iterator protocol despite one of them literally being called 'Iterator' 😄.

I still think they can be useful -- it's nice to have a built-in API to give you the confidence you're not making a significant performance blunder. I wouldn't mind keeping them around with some improvements.

If you compare [javascript] NodeIterator and [javascript] querySelector on Stack Overflow the difference is quite stark.

I'm not convinced the comparison is valid here. If you need to iterate over a flat array of elements, querySelectorAll is a fine alternative. But NodeIterator isn't limited to HTMLElement -- it also can include text nodes, Attr nodes, CDATA, etc. The Text node use case in particular makes this API necessary in some scenarios.

I mean the alternative is to just write the iteration code yourself, right?

This is a fair point -- a simple lazy tree-walker can be built with a recursive generator:

function* walkNodeTree(root) {
  yield root;
  for (const node of root.childNodes) yield* walkNodeTree(node);
}

This isn't hard, but it's also not completely trivial. Add on support for skipping individual nodes / rejecting node subtrees and the logic starts to get a little more complex. And I doubt there are many browsers that will tail-call optimize a generator, so you'll probably want to add a trampoline.... there's plenty of subtle considerations here.

I think the first step here would be demonstrating why it's so bad to write loops in JavaScript.

It seems to me that the primary advantage of the original feature request (adding iterator support) would actually be that it makes these APIs easier to use with loops (for (const node of iterator {...}). So this request is an argument for loops, not against them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

7 participants