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

Drop .asIndexedPairs from this proposal or replace it with .entries #143

Closed
btoo opened this issue Jul 18, 2021 · 20 comments · Fixed by #183
Closed

Drop .asIndexedPairs from this proposal or replace it with .entries #143

btoo opened this issue Jul 18, 2021 · 20 comments · Fixed by #183

Comments

@btoo
Copy link
Contributor

btoo commented Jul 18, 2021

Assuming the iterator .map has about the same function signature as the array .map:

interface Iterator<T, TReturn = any, TNext = undefined> {
    next(...args: [] | [TNext]): IteratorResult<T, TReturn>;
    return?(value?: TReturn): IteratorResult<T, TReturn>;
    throw?(e?: any): IteratorResult<T, TReturn>;

    map<U>(callbackfn: (value: T, index: number, iterator: Iterator<T, TReturn, TNext>) => U, thisArg?: any): Iterator<U, TReturn, TNext>
}

.asIndexedPairs seems like an overengineering when iterator.map((value, index) => [value, index]) would effortlessly achieve the same result, not to mention be more versatile


if we must keep .asIndexedPairs, can it be renamed as .entries for interoperability with virtually everything that implements the iterator protocol?

@Jack-Works
Copy link
Member

.entries looks good

@callionica
Copy link

I'm here to support renaming asIndexedPairs to entries

@tjoskar
Copy link

tjoskar commented Jul 21, 2021

If map (and all other helper methods) will call the given function with an index I do not see any use case of asIndexedPaires. But if I understand the current documentation correct there is no such thought. And after looking at Rust, there is no index argument there either (without calling enumerate first) and I guess that is because of performance reasons?

So if there is a good reason to not include the index in all methods; I will also vote for renaming asIndexedPaires to entries to align with Array.prototype.entries.

@btoo
Copy link
Contributor Author

btoo commented Jul 21, 2021

not sure if what @ljharb said about keeping a hypothetical iterator.slice "exactly" like arrayOrString.slice in #131 (comment) applies to iterator.map (or almost every other helper method in this proposal, for that matter), but my vote is certainly for keeping the APIs as consistent as possible, again for interoperability!

wrt performance, I believe including the index only costs constant space (and linear runtime if you count variable assignment). negligible if you ask me

@zloirock
Copy link
Contributor

zloirock commented Jul 21, 2021

.entries LGTM

@NotWearingPants
Copy link

NotWearingPants commented Jul 21, 2021

I vote for keeping asIndexedPairs, its readability outweighs having a name consistent with object.entries.

Also of course anyone can just iterator.map((value, index) => [value, index]) but this use case is common enough to have it built in.

@devsnek
Copy link
Member

devsnek commented Jul 21, 2021

fwiw I picked the name asIndexedPairs under the assumption that it would be renamed. This may have even been discussed in committee but it was long enough ago that I don't remember.

@codehag
Copy link
Collaborator

codehag commented Aug 18, 2021

I will check if there are issues with this, if not I will fold it in. thanks for the suggestion.

@iwikal
Copy link

iwikal commented Oct 22, 2021

To throw in another option to bikeshed about: in npm itertools, ballvalve, Python, Rust std Iterator, and Rust itertools, this method is named enumerate. But entries makes a lot of sense as well. My only objection to asIndexedPairs is that this is such a commonly used method, it warrants a shorter and more easily typed name. In general, I get a feeling that since these methods are meant to be used in a way similar to the unix philosophy of chaining many simple operations together, it seems that most APIs tend towards using shorter names.

@bergus
Copy link

bergus commented Oct 24, 2021

I don't understand what "interoperability" refers to in

can it be renamed as .entries for interoperability with virtually everything that implements the iterator protocol?

None of the standard iterators that implement the iterator protocol have an .entries() method so far. If you meant the iterable protocol, there are quite a few objects that don't have an .entries() method (like generators) or whose .entries() method does not return tuples where the first member is an integer (notably Map and Set). The only things where .entries() returns index-value pairs are arrays, and there's no reason to assume that all iterators are array-like. I therefore heavily disagree with naming this method "entries".

If .asIndexedPairs() is too long, just make it .indexed(); although I also like enumerate that is common in many other languages.

@devsnek
Copy link
Member

devsnek commented Oct 24, 2021

i would personally prefer enumerate but people were not fans of it.

@zloirock
Copy link
Contributor

.enumerate here is absolutely unintuitive.

@ljharb
Copy link
Member

ljharb commented Oct 24, 2021

entries does not connote that the key is an integer, that's just the way arrays happen to work. An entry is just a tuple-2 of "key" and "value", so I think the name applies here. enumerate is definitely too general, and also implies object properties, especially since that's what Reflect.enumerate did before it was removed.

@bergus
Copy link

bergus commented Oct 24, 2021

@ljharb

entries does not connote that the key is an integer

And that's exactly the issue I have, the reason why I think the name does not apply here. I'd want something that emphasises adding indices to the values in the iterator. Key-value tuples are fine for the entries of collections, but I don't think of an iterator as containing entries.

@bakkot
Copy link
Collaborator

bakkot commented Oct 24, 2021

entries would make a lot more sense if there were a natural way to use the "key" to get the value, as there is for all other APIs using that name. Since there isn't, it seems like an odd choice. An iterator is a sequence, not a collection.

.indexed() and .enumerate() both SGTM personally. Regarding enumerate in particular, I think the analogy to Python's enumerate is going to occur to far more people than the late Reflect.enumerate would.

@ljharb
Copy link
Member

ljharb commented Oct 24, 2021

@bergus Object.entries also means "entries" and offers no guarantees that the key is an integer, so I don't think your issue actually holds.

However, #143 (comment) is compelling. .indexed makes sense to me - enumerate doesn't imply there's necessarily an index, just an ordering.

@js-choi
Copy link

js-choi commented Oct 24, 2021

Clojure uses indexed. (Actually Clojure has two indexed functions, keep-indexed and map-indexed, in an effort to minimize allocation/GC of intermediate entry objects, but that could reasonably seen as overkill for JavaScript, and .map already supplies indices.)

@iwikal
Copy link

iwikal commented Oct 25, 2021

Confusing iterator.entries() with Map entries does seem like a real risk.

let users = new Map()
users.set('alice', new User())
users.set('bob', new User())

users.entries().entries()

Here, entries means something entirely different on each invocation, but with deceptively similar return types. With some indirection, one could easily get the wrong result without necessarily crashing and revealing the mistake.

function saveUsersToLocalStorage(users) {
  const data = {}
  for (const [name, user] of users.entries()) {
    data[name] = user.serializable()
  }
  localStorage.users = JSON.stringify(data)
}

let users = new Map()
users.set('alice', new User())
users.set('bob', new User())
saveUsersToLocalStorage(users.entries())

Unless you're using some sort of static type checking, this could be very hard to track down. indexed is less ambiguous, as is enumerate.

@hax
Copy link
Member

hax commented Jul 4, 2022

I don't like entries, it implies values/keys and I don't think we would like to add such helpers.

I also worry about potential confusion with enumerate and enumeration (enum), and some programming language use enumerate in the meaning of iterate (especially C#).

So I vote for indexed. If zip was included in the proposal, zipWithIndex is also an option.

michaelficarra added a commit that referenced this issue Jul 6, 2022
Co-authored-by: Michael Ficarra <mficarra@shapesecurity.com>
@btoo
Copy link
Contributor Author

btoo commented Jul 11, 2023

#211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.