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

Why does list get converted into iterable? #684

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

Why does list get converted into iterable? #684

OliverJAsh opened this issue Nov 2, 2015 · 8 comments

Comments

@OliverJAsh
Copy link

@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
Copy link

@tomwidmer 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
Copy link
Author

@OliverJAsh 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
Copy link

@myitcv 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
Copy link
Author

@OliverJAsh 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
Copy link
Author

@OliverJAsh 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
Copy link

@myitcv 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
Copy link
Author

@OliverJAsh OliverJAsh commented Nov 25, 2015

Thanks @myitcv, that helps!

@leebyron
Copy link
Collaborator

@leebyron 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.