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

for-of does not work with some DOM collections when target is ES6 #2695

Closed
Arnavion opened this issue Apr 9, 2015 · 42 comments
Closed

for-of does not work with some DOM collections when target is ES6 #2695

Arnavion opened this issue Apr 9, 2015 · 42 comments
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this

Comments

@Arnavion
Copy link
Contributor

Arnavion commented Apr 9, 2015

Update: This issue is now only for the following collections:

  • MediaList
  • StyleSheetList
  • CSSRuleList

Originally it was about NodeList as well, but that was fixed (along with DOMTokenList) in #3393


Original:

var paragraphs = document.querySelectorAll("p");
for (let p of paragraphs) {
    console.log(p);
}
foo.ts(2,15): error TS2488: The right-hand side of a 'for...of' statement must have a '[Symbol.iterator]()' method that returns an iterator.

Seems it just needs an update to lib.es6.d.ts to add the [Symbol.iterator()] to all the collections that have an indexer and length property. The ones I found that have it in Nightly are below. The rest were either IE-only or didn't exist. Removed my list since it seems FF has more than the specs allow. See zloirock/core-js#137 (comment) for a more accurate list.

@mhegazy mhegazy added Bug A bug in TypeScript ES6 Relates to the ES6 Spec labels Apr 9, 2015
@mhegazy mhegazy added this to the TypeScript 1.5 milestone Apr 9, 2015
@mhegazy mhegazy self-assigned this Apr 9, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Apr 9, 2015

@zhengbli would this be covered in your change?

@Arnavion
Copy link
Contributor Author

Arnavion commented Apr 9, 2015

(This would also require making two versions of dom.d.ts - for ES5 and ES6.)

@mhegazy
Copy link
Contributor

mhegazy commented Apr 9, 2015

yup. that is correct.

@djarekg
Copy link

djarekg commented Apr 30, 2015

This should also be supported in ES3/ES5 since the emitted loop is just looping over the indexes.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 30, 2015

iterators relay on symbols and these are defined in ES6. I think what we need is to allow iterating ArrayLike in ES3/ES5 (#2862).

@Arnavion
Copy link
Contributor Author

Arnavion commented May 1, 2015

@djarekg That is not correct. It can also require converting NodeList to an array first (with slice) to handle the livelist getting modified during iteration. See #2696

@NekR
Copy link

NekR commented May 31, 2015

Please do not forget to include in this list TouchList interface. Thanks.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2015

@saschanaz has added support for:

  • NodeList
  • NodeListOf
  • DOMTokenList

The other interfaces do not seem to be iterable in the spec. @Arnavion any ideas?

@Arnavion
Copy link
Contributor Author

Arnavion commented Jun 9, 2015

They're not iterable<> but they are ArrayClass, eg CSSRuleList. That means their prototype is supposed to be an Array instead of Object and so they have all the properties of Array including [Symbol.iterator], although no browser has actually gone that far, not even Nightly.

NodeList is also supposed to be ArrayClass instead of iterable<>, but Chrome had problems with it and (temporarily?) abandoned it. FF has an open bug to convert NodeList from iterable to ArrayClass but so far nothing has happened. I don't know what browsers' plan for these "ArrayClass" interfaces - I would guess it's likely that all ArrayClass interfaces will eventually be iterable even if they aren't full-blown Arrays.

The array iterator Array.prototype[Symbol.iterator] is also usable for all DOM collections, and this is what Nightly does. For example, CSSRuleList.prototype[Symbol.iterator] === Array.prototype[Symbol.iterator] is true in Nightly. This is intentional - ArrayIterator is supposed to work with ArrayLikes. So even in browsers where DOM collection prototypes don't already have a Symbol.iterator property, assigning it with, say CSSRuleList.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];, works and makes them iterable with for-of. This would be an argument for TS to allow for-of for these types. The user just needs to add a polyfill to assign the [Symbol.iterator] property on all the DOM list prototypes they're interested in iterating with for-of. But these will have to be custom polyfills - I don't know any polyfill library that adds Symbol.iterator properties to all DOM collections. Babel (core-js) does it but only for NodeList.

On the other hand, Array.from() works for them since they are array-likes, and even in TS they implicitly extend ArrayLike<T> so Array.from()'s signature is not a problem. So that's an argument against allowing for-of for these types in TS.

So basically, from the user's point of view, either they use a polyfill for Array.from, and use Array.from to convert all their DOM collections before using for-of. This requires no change from TS, and such a polyfill already exists. (Then there was no point to adding [Symbol.iterator] to NodeList either, but anyway...)

Or, TS allows for-of with all DOM collections, and the user uses a polyfill that adds [Symbol.iterator] properties to all those collections. This does require change from TS, and such a polyfill doesn't already exist.

I don't have an opinion either way.

@mhegazy mhegazy modified the milestones: TypeScript 1.7, TypeScript 1.6 Jul 1, 2015
@dead-claudia
Copy link

@Arnavion

Neither NodeList and DOMTokenList are supposed to be Array subclasses. They are only supposed to be iterable, as in:

  • JavaScript:

    NodeList.prototype[Symbol.iterator] = function* () {
       // ...
    }
  • Python:

    class NodeList:
      # ...
      def __iter__(self):
        # ...
  • Java:

    import java.util.*;
    public class NodeList implements Iterable<Node> {
      // ...
      Iterator<Node> iterator() {
        // ...
      }
    }
  • Scala:

    import scala.collection.{Iterable, Iterator}
    class NodeList extends Iterable[Node] {
      // ...
      def iterator: Iterator[Node] = // ...
    }

@Arnavion
Copy link
Contributor Author

Arnavion commented Jul 8, 2015

@IMPinball Yes they are supposed to be, per the spec. I gave links for the other CSSOM objects in the previous post, and the link for NodeList is here where you can also see it's described as ArrayClass.

I already explained that no browser has actually implemented them as ArrayClass.

Edit: I see you're specifically talking about the whatwg's spec. Sure, that's the one browsers are more likely to implement anyway. However also note that whether they are ArrayClass or not is not relevant to this issue.

@dead-claudia
Copy link

@Arnavion I see what you're talking about in the edit...This bug is actually about the following problem:

interface Foo {
  *[Symbol.iterator](): Iterator<any>;
}

declare class Bar {
  *[Symbol.iterator](): Iterator<any>;
}

declare let baz: {
  *[Symbol.iterator](): Iterator<any>;
};

declare let foo: Foo;
let bar = new Bar();

for (let _ of foo) {} // Fail?
for (let _ of bar) {} // Fail?
for (let _ of baz) {} // Fail?

Does that repro in any of the three cases? I don't have a dev version handy to test it.

@Arnavion
Copy link
Contributor Author

Arnavion commented Jul 9, 2015

Without the * (asterisks are not needed for declarations), yes all of those compile, as they're supposed to. This bug is about adding Symbol.iterator properties to the DOM types in the first place.

@dead-claudia
Copy link

Okay...that is probably trivial

@dead-claudia
Copy link

Unless it's something else...the declarations already exist.

@dead-claudia
Copy link

That file was merged on June 5.

@Arnavion
Copy link
Contributor Author

Arnavion commented Jul 9, 2015

Please read this thread...

@dead-claudia
Copy link

It type-checks for me with the master branch with --target es6. I just checked on my machine.

@mhegazy mhegazy modified the milestones: Future, TypeScript 2.1 Sep 29, 2016
@mhegazy mhegazy added the Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript label Apr 20, 2017
@mhegazy mhegazy modified the milestones: Community, Future Apr 20, 2017
@mhegazy mhegazy added Help Wanted You can do this and removed ES6 Relates to the ES6 Spec labels Apr 20, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2017

PRs welcomed. The change should be in https://github.com/Microsoft/TypeScript/blob/master/src/lib/dom.iterable.d.ts

@saschanaz
Copy link
Contributor

@mhegazy Please consider microsoft/TypeScript-DOM-lib-generator#235 too ;)

@KeithHenry
Copy link

This has had no activity for a year - is it still considered a problem?

The issue still appears to be extant:

const collection: HTMLCollection = getCollection();
const arrayCopy = [...collection]; // (TS) Type must have a '[Symbol.iterator]()' method that returns an iterator.

@saschanaz
Copy link
Contributor

@KeithHenry I've been actively fixing wrong types on https://github.com/Microsoft/TSJS-lib-generator, I think this also will be fixed within months, or you can send your own PR there.

@KeithHenry
Copy link

@saschanaz cheers, but that already has HTMLCollection configured as iterable. What else needs to change?

@saschanaz
Copy link
Contributor

saschanaz commented Apr 24, 2018

@KeithHenry That file should replace lib.dom.iterable.d.ts but the upstream does not yet support methods including entries(). Coming Soon™️, though!

@nevercast
Copy link

What do you mean by upstream does not support, do you mean iterators, in general, are broken? or is this purely a type definition issue?

@saschanaz
Copy link
Contributor

Ah, by upstream I mean this one.

@saschanaz
Copy link
Contributor

This can be closed now as lib.dom.iterable.d.ts have iterators for MediaList, StyleSheetList, CSSRuleList, and many more.

@thany
Copy link

thany commented Feb 5, 2019

This problem still exists for me when trying to iterate over NodeListOf<Element>. A NodeList is always iterable. Period.

Some code:

const triggers = element.querySelectorAll('.trigger');
for (const trigger of triggers) {
    console.log(trigger);
}

The error is on triggers in VS Code, using typescript 3.2.2.

The error:

[ts] Type 'NodeListOf<Element>' is not an array type or a string type. [2495]

Technically correct. A NodeList is indeed neither an array nor a string. But for..of works on a great many more kinds of objects, including NodeList.

The above code compiles and works fine. So why complain about something that plainly isn't the case?

@saschanaz
Copy link
Contributor

@thany Ensure the target is ES6 or higher. Currently TS does not support for-of on general iterable objects when the target is ES5/ES3.

@RyanCavanaugh
Copy link
Member

Or enable --downlevelIteration

@thany
Copy link

thany commented Feb 11, 2019

First of all, why does TS need to support it at all? It just needs to output the transpiled JS and let JS handle whether the object can be looped over or not.

Apart from that, NodeList is always iterable, in every browser. Some require a indexed for-loop, newer browser suport for..of. But either way the object can be looped over perfectly fine.

If the target is ES5, produce a classic for-loop.
If the target is ES6 or higher, produce a for..of loop.

I don't see how this is complicated in any way.

@saschanaz
Copy link
Contributor

@thany Have you tried --downlevelIteration (introduced in TS2.3, I forgot about it 😅)? I think that should work.

@thany
Copy link

thany commented Feb 12, 2019

I've enabled downlevelIteration (in our tsconfig.json) which doesn't produce any difference. The code still builds and works fine, but I still get the error in VS Code.

Also since the update, VS Code has switched to TS 3.3.1, in case you didn't know.

@saschanaz
Copy link
Contributor

saschanaz commented Feb 12, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests