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 with map.keys() gives TS2495 #11209

Closed
davidreher opened this issue Sep 28, 2016 · 18 comments
Closed

for-of with map.keys() gives TS2495 #11209

davidreher opened this issue Sep 28, 2016 · 18 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@davidreher
Copy link

TypeScript Version: 2.0.3

Code

let map = new Map<string, string>();
map.set('hello', 'world');

for (let key of map.keys()) {
    console.log(key);
}

Expected behavior:
It should not throw.

Actual behavior:
It throws error TS2495: Type 'IterableIterator<string>' is not an array type or a string type.

tsconfig.json:

{
  "compilerOptions": {
    "experimentalDecorators": true,
    "lib": [
      "dom",
      "es7",
      "scripthost"
    ],
    "moduleResolution": "node",
    "noImplicitAny": false,
    "preserveConstEnums": true,
    "pretty": true,
    "removeComments": false,
    "sourceMap": true,
    "target": "ES5"
  }
}

cc: @Mischi

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 28, 2016
@RyanCavanaugh
Copy link
Member

This is correct because we don't have any correct way to downlevel your for / of into a for loop that works in a pure ES5 environment. Note that we would emit this code, which doesn't work:

for (var _i = 0, _a = map.keys(); _i < _a.length; _i++) {
    var key = _a[_i];
    console.log(key);
}

The assumption is that if you're saying --target ES5, there aren't the intrinsic ES6 functions available to produce the proper iterating loop.

@davidreher
Copy link
Author

Hmm, k thought with the help of core-js this should work as I expected :D

@kitsonk
Copy link
Contributor

kitsonk commented Sep 28, 2016

You are crossing the syntactical/functional boundary here. TypeScript deals with syntactical features of the language, which means for ES5, for for ... of it can only down-emit for structures that it "knows" about, versus assuming that there is some sort of iterable that it can deal with, which is ArrayLike which is an object with .length which allows indexed based accessed to it values.

You can make a functional fill that would replicate the functionality and deal with any iterable, without needing to use syntactical down-emitting. We have one in Dojo 2 in our dojo-shim package and would look something like this:

import { forOf } from 'dojo-shim/iterator';

let map = new Map<string, string>();
map.set('hello', 'world');

forOf(map.keys(), (key) => {
    console.log(key);
});

(We also have a Map shim written in TypeScript that offloads to native too...)

@Mischi
Copy link

Mischi commented Sep 29, 2016

For comparsion this is what babel emits for the following snippet. Would it not be possible to emit something similiar in TypeScript?

let map = new Map();
map.set('hello', 'world');

for (let key of map.keys()) {
    console.log(key);
}

transpiled

'use strict';

var map = new Map();
map.set('hello', 'world');

var _iteratorNormalCompletion = true;
var _didIteratorError = false;
var _iteratorError = undefined;

try {
    for (var _iterator = map.keys()[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
        var key = _step.value;

        console.log(key);
    }
} catch (err) {
    _didIteratorError = true;
    _iteratorError = err;
} finally {
    try {
        if (!_iteratorNormalCompletion && _iterator.return) {
            _iterator.return();
        }
    } finally {
        if (_didIteratorError) {
            throw _iteratorError;
        }
    }
}

@Mischi
Copy link

Mischi commented Sep 29, 2016

Another thought. As we specify "es7" in lib, TypeScript knows that APIs like Symbol are available at runtime and could emit the appropriate code.

@davidreher davidreher reopened this Sep 29, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Sep 29, 2016

It is not about the type or the existence of Symbol. it is about the output code. as you can see the output above is significantly different from the original code. there are multiple dispatches, and object allocations in cases of arrays. there is also the try/finally blocks that impact the engines ability to optimize the code. also keep in mind we can not just emit this for maps, we have to emit this for all for .. of, or spread operators. that also applies to things like for (x of [1, 2, 3]) since we do not know the type of this object at runtime.

TypeScript always strives to emit idiomatic and performant code. for this we have chosen to only allow transformation of for ..of on arrays only. this gives most of the users most of the benefit.

@RyanCavanaugh
Copy link
Member

This emit is meaningless -- if you have an ES6 runtime, you should just say --target ES6 instead of --target ES5. We're not going to invest a bunch of time supporting runtimes which support some arbitrary 43% of ES6; these are rare and getting rarer.

@davidreher
Copy link
Author

ok, thx again for clarification

@mischkl
Copy link

mischkl commented Oct 18, 2016

This issue is causing serious pain for us. We have the requirement to support ES5-capable browsers such as IE11 and this isn't going away anytime soon. Adding Babel to the mix adds serious complication to our build process and makes the source maps essentially useless. So as a workaround we thought we would add Babel into the mix only for the production build.

So fine, let's naively "turn on" ES6 as the target in tsconfig.json, and use ES6 directly for our development build, since there we can ensure that everyone's using the latest Chrome. So... after removing core-js type definitions in order to get it to build at all, the modules are amazingly enough tolerated by Webpack 2. BUT the resulting bundle doesn't actually work in the latest Chrome. It's fine and good to say "in theory it's easy, just target ES6", but the reality seems to be another matter entirely.

So here's my 2 cents: This is something Babel can do and TypeScript can't. "Philosophy" here or there, that's the simple truth, and tbh, people who are targeting ES5 probably couldn't care less exactly what the final output looks like, they just want it to work and have good source maps support. No to mention, I think the philosophy "just works regardless of compilation settings" makes a lot more sense than the current rationalization.

If you're not willing to invest in a direct solution, at the very least there should be a path of least resistance to get these ES6 features working with ES5 as a target -- i.e. proper documentation and perhaps a demo project setup -- because this is a major use case that's not likely to go away anytime soon.

@davidreher davidreher reopened this Oct 18, 2016
@Arnavion
Copy link
Contributor

Are you asking for a sample project that has TS source and uses Babel + Webpack to build the final bundle? Google finds some but they predate Babel 6 / Webpack 2 so perhaps a newer template is warranted.

@mischkl
Copy link

mischkl commented Oct 18, 2016

I'm asking for, at a minimum, documentation that is maintained. If this includes a sample project that should be maintained, too. And it needs to include reliable source maps support.

Even more welcome would be a TypeScript-approved shim/polyfill that "just works" to support all of the most-used ES6 features - and documentation on how to integrate it. Up until now I had assumed that this was true of core-js, but it seems that this is not the case after all, considering that iterating over Maps and Sets is not possible.

Essentially the core of my message is that when it comes to supporting ES6 collections, TypeScript is leaving users who want to target ES5 in the cold. This is a serious oversight IMHO.

@aluanhaddad
Copy link
Contributor

TypeScript always strives to emit idiomatic and performant code. for this we have chosen to only allow transformation of for ..of on arrays only. this gives most of the users most of the benefit.

@mhegazy If people are writing for..of to loop over arrays, the kind of transformation that babel emits is certainly awful.
I personally never write for..of loops over arrays because I don't find it valuable. As soon as you want to project or filter elements it becomes an imperative and verbose exercise. I prefer .forEach, in all but the most performance critical sections. Obviously every user is different but the ability to iterate over Iterables, and IterableIterators would be a new feature, not simply a new, and little improved, way of writing a for loop so I find it frustrating because this is the only reason I would use for..of in the first place.

I understand the reasoning behind not supporting this, but I don't think for..of adds any value to ES5 without it.

@screendriver
Copy link

Just for clarification: I target ES5 because I have to support older and mobile browsers. I included core-js in my webpack build so I can use any of the new ES2015 features like Promises and Map (my tsconfig.json includes something like this for that: "lib": [ "dom", "es5", "es2015" ] Now I can use Promises and Map in my TypeScript code).

So my question is: with the latest TypeScript compiler 2.1.1 I can use async and await on my Promises although I target ES5 but I can't use for..of loops for a Map?

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 30, 2016

This is being resolved by #12346 I'm very excited

@smac89
Copy link

smac89 commented May 22, 2017

The fix for me was to do this:

let map = new Map<string, string>();
map.set('hello', 'world');

for (let key of Array.from(map.keys())) {
    console.log(key);
}

This works even when targeting es5 btw

@TazmanianD
Copy link

@smac89 One drawback with your approach is that it introduces an extra O(n) copy operation. The Array.from has to construct a whole new array and copy all the values from the iterator into it. If performance is a concern, you should be aware of this.

@dylanpyle
Copy link

dylanpyle commented Jan 5, 2018

@smac89 @TazmanianD I think we can cheat a bit and use the second argument to Array.from to avoid the second iteration:

const map = new Map<string, number>();
map.set('a', 1);
map.set('b', 2);

Array.from(map.values(), (value: number) => {
  console.log(value);
});

edit: Didn't see the comment @ #11209 (comment), that's great news.

@aluanhaddad
Copy link
Contributor

@dylanpyle yeah, this has worked since [email protected]

Just set --downlevelIteration and go to town.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests