-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
PSA: potential lib breaking change after iterator-helpers proposal #54481
Comments
The biggest issue I see is that we can't properly model the type of the native declare abstract class Iterator<T> {
constructor();
abstract next(value?: undefined): IteratorResult<T, void>;
// ... iterator helper instance methods ...
static from<T>(obj: Iterable<T> | TheCurrentIteratorType<T>): Iterator<T>;
// ... iterator helper static methods ...
} While we can model the abstract constructor using an interface IterableIterator<T> extends Iterator<T> {
abstract next(value?: undefined): IteratorResult<T, void>;
// ^^^^^^^^ this doesn't matter for implementing interfaces. it is
// only applied when tied to an `abstract new` signature.
[Symbol.iterator](): IterableIterator<T>;
// ... iterator helper instance methods ...
}
interface IteratorConstructor {
abstract new<T>(): IterableIterator<T>;
// ^^^^^^^^ opt-in to caring about the `abstract`-ness
// of the interface.
readonly prototype: IterableIterator<any>;
from(obj: Iterable<T> | Iterator<T>): IterableIterator<T>;
// ... iterator helper static methods ...
}
declare var Iterator: IteratorConstructor; |
I’m confused; Iterator.prototype.next exists, why would you need to define it yourself? |
ahhh, you're right, in this case it's IteratorHelperPrototype.next that the proposal adds. |
It could be renamed to |
Now that iterator helpers are shipped in Chrome and Edge, meaning it will also ship in the next NodeJS release this month (and there are polyfills available too for other engines that currently have it behind experimental flags) it would be really nice if TypeScript could tackle typing the iterator helpers. Given the current naming conflict, it is difficult to write custom typings to fill in the gap in userland (and I haven't found any successful attempts in DT or polyfill packages). |
A concrete proposal to maybe get the ball rolling:
This is a breaking change for anyone using the The main way that this would break is that places which declared to need an |
The main way this would break is that any existing package that serves an interface IteratorConstructor {
// ...static methods
}
declare var Iterator: (abstract new <T>() => IterableIterator<T>) & IteratorConstructor; |
That's what I was saying in the second half of the bit you quoted, yeah. But that wouldn't actually cause any existing code to fail to compile (except in quite obscure cases which depend on non-existence of
Another option would be to stick with declare abstract class __TheActualIteratorBuiltin<T> implements IterableIterator<T> {
abstract next(value?: undefined): IteratorResult<T, void>;
[Symbol.iterator]: () => NativeIterator<T>;
map<R>(fn: (v: T) => R): NativeIterator<T>;
// etc
}
declare var Iterator: typeof __TheActualIteratorBuiltin;
type NativeIterator<T> = __TheActualIteratorBuiltin<T>; Incidentally, with any of these approaches, how do you update the type for Generator so that it properly inherits these new methods, without just copying them all over? Right now generators implicitly implement |
I'm also fine with calling this
We would just do |
We've generally discouraged implementing them directly in the past to try to set the expectation that |
Yeah, that's what I was going for with the
There's 77 files on DT which match |
I may have a solution to use |
Opened #58222 as a draft with the |
FYI as of today iterator helpers are shipped in NodeJS (v22) |
What’s the next step to enable iterator helpers so we can use them in Node? |
lib Update Request
Configuration Check
My compilation target is
ESNext
and my lib isthe default
.Missing / Incorrect Definition
The TS lib defs for
Iterator
,Iterable
, andIterableIterator
treat them as if they are all fictitious interfaces (a.k.a. protocols). The lib def's inheritance structure looks like this:However, while
Iterable
and arguablyIterableIterator
are protocols,Iterator
is an actual JS entity. See MDN docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator This has some practical issues, because what TS terms asIterableIterator
is the actual JSIterator
class, but what TS terms asIterator
is what JS regards as "iterator-like" or "iterator protocol". For example, the def fornew Set().values()
says it returns anIterableIterator
, while in fact it should return an instance of the JSIterator
instance.This becomes more of an issue after the iterator-helpers proposal, which is going to expose
Iterator
as an actual global that people can use asextends
. For example:Under the current lib def,
#MyClassIterator
won't be seen as iterable by TS, because it extendsIterator
, notIterableIterator
.On the other hand, if we change the definition of
Iterator
to say it has[Symbol.iterator]
(mirroring what happens in JS), then it will break code of the following kind:Of course you could assume that almost all userland iterators are built-in and therefore inherit from
Iterator
and have@@iterator
, but it's a breaking change nonetheless.The question thus arises that when iterator-helpers lands, where should we put the new definitions on. Right now, I'm using core-js to polyfill these methods, and when writing declarations, I chose to put them on
IterableIterator
, so thatnew Set().values()
works:...but this is suboptimal, for obvious reasons (it means the
Iterator
class doesn't actually have these methods).Sample Code
Documentation Link
The text was updated successfully, but these errors were encountered: