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

Why does list get converted into iterable? #684

Closed
OliverJAsh opened this Issue Nov 2, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@OliverJAsh

OliverJAsh commented Nov 2, 2015

This might be a silly question.

How come when I map over a list it gets converted into an iterable? This means I have to litter my code with toList everywhere, like so:

List([1,2]).map(x => x+1).toList().interpose('foo');

Thanks again for the awesome work!

@tomwidmer

This comment has been minimized.

tomwidmer commented Nov 3, 2015

It doesn't, in your example toList() is redundant. The first line of the docs for map states: "Returns a new Iterable of the same type". Remember List extends Iterable, and the return type is given as Iterable to avoid having to write separate map documentation for all the different collection types. However, Map.map returns a Map, List.map returns a List, etc.

@OliverJAsh

This comment has been minimized.

OliverJAsh commented Nov 3, 2015

Ah, but I get a compile error when using TypeScript:

Immutable.List(['foo'])
    .map(x => x)
    .interpose(5)

Error:

main.ts(27,6): error TS2339: Property 'interpose' does not exist on type 'Iterable<number, string | number>'.
@myitcv

This comment has been minimized.

myitcv commented Nov 4, 2015

#634 and Microsoft/TypeScript#4967 are related.

Large parts of the type definition can be improved following the merging of Microsoft/TypeScript#4910 however the issue with map remains as far as I can tell (see the comments in Microsoft/TypeScript#4967 in particular)

Whatever the solution here, it raises the question of how we improve the type definition whilst maintaining some degree of backwards compatibility. I can potentially foresee a situation where we end up with multiple typings with suffixes that correspond to TypeScript compiler versions...

In any case, the immediate solution here is to simply assert the type:

(<List<number>>List([1,2]).map(x => x+1)).interpose('foo')

because we know the underlying implementation actually returns a container of the same type, List<number> in this situation.

@OliverJAsh

This comment has been minimized.

OliverJAsh commented Nov 13, 2015

I am using type assertions to work around this, but how come this doesn't work?

(<List<number | string>>List([1,2]).map(x => x+1)).interpose('foo')
@OliverJAsh

This comment has been minimized.

OliverJAsh commented Nov 13, 2015

Or this:

const x = <List<number>>List([1, 2]).map(x => x + 1);
x.map(x => x + 1).interpose('foo')
@myitcv

This comment has been minimized.

myitcv commented Nov 13, 2015

Breaking apart your first example according to precedence of operators:

List([1,2]) // has type List<number>, inferred from the array of numbers you provide
List([1,2]).map(x => x+1) // therefore has type Iterable<number, number>, per previous comments about map signature

The type assertion has the weakest precedence hence these two are equivalent:

<List<number | string>>List([1,2]).map(x => x+1)
<List<number | string>>(List([1,2]).map(x => x+1))

which is why you get a compile error in both cases.

Given your map function (which appears to only work on number), I'm not entirely sure what you intended here... but hopefully the comments about precedence help steer you in the right direction.

Some other comments:

let a = List([1,2]); // a has type List<number>
let b = List<number>([1,2]); // b has type List<number>
let c = List<number | string>([1,2]); // c has type List<number | string>
let d = <List<number | string>>List([1,2]); // d has type List<number | string>
@OliverJAsh

This comment has been minimized.

OliverJAsh commented Nov 25, 2015

Thanks @myitcv, that helps!

@leebyron

This comment has been minimized.

Collaborator

leebyron commented Mar 4, 2017

Fixed in master, will be released soon

@leebyron leebyron closed this Mar 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment