Skip to content

Conversation

elimanzo
Copy link
Contributor

@elimanzo elimanzo commented Sep 8, 2024

based off of MDN

closes #343

@elimanzo elimanzo self-assigned this Sep 8, 2024
@elimanzo
Copy link
Contributor Author

elimanzo commented Sep 8, 2024

@miorel I think the snapshots are failing because of the declare class. I'm currently getting

Only interface declarations are currently supported in module declarations, got: ClassDeclaration

@elimanzo elimanzo marked this pull request as ready for review September 8, 2024 21:50
@elimanzo elimanzo requested a review from miorel as a code owner September 8, 2024 21:50
miorel added a commit that referenced this pull request Sep 10, 2024
…419)

Should help unblock #417 by not requiring everything inside a `declare global` block to be an interface.
@elimanzo elimanzo force-pushed the em-343-iterator-from branch from 78e3ada to 0b65684 Compare September 14, 2024 17:59
@code-chronicles-code code-chronicles-code deleted a comment from elimanzo Sep 15, 2024
@elimanzo elimanzo force-pushed the em-343-iterator-from branch from 5d60da1 to 319cf8b Compare September 17, 2024 02:32
@elimanzo elimanzo requested a review from miorel September 17, 2024 02:40
@elimanzo elimanzo force-pushed the em-343-iterator-from branch from 980c625 to 319cf8b Compare September 17, 2024 02:50
Comment on lines 21 to 30
if (typeof iterator === "object" && Symbol.iterator in iterator) {
return iterator as IterableIterator<T>;
} else {
return {
...iterator,
[Symbol.iterator]() {
return this;
},
} as IterableIterator<T>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the implementation of .toIterable here? I'm hoping we can call it with a custom this, similar to what we were discussing this weekend.

Suggested change
if (typeof iterator === "object" && Symbol.iterator in iterator) {
return iterator as IterableIterator<T>;
} else {
return {
...iterator,
[Symbol.iterator]() {
return this;
},
} as IterableIterator<T>;
}
return iteratorPrototype.toIterable.call(iterator);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused did we want the custom annotation kind of like

declare global {
  interface IteratorConstructor {
    from<T>(
      this: Iterator<T>, // this context
      object: Iterator<T> | Iterable<T> | { next(): IteratorResult<T> },
    ): IterableIterator<T>;
  }

  const Iterator: IteratorConstructor;
}

Also while returning
return iteratorPrototype.toIterable.call(iterator);
it seems that I need to assert as IterableIterator<T> because of the type error

Type 'IterableIterator<unknown>' is not assignable to type 'IterableIterator<T>'.
  The types returned by 'next(...)' are incompatible between these types.
    Type 'IteratorResult<unknown, any>' is not assignable to type 'IteratorResult<T, any>'.
      Type 'IteratorYieldResult<unknown>' is not assignable to type 'IteratorResult<T, any>'.
        Type 'IteratorYieldResult<unknown>' is not assignable to type 'IteratorYieldResult<T>'.
          Type 'unknown' is not assignable to type 'T'.
            'T' could be instantiated with an arbitrary type which could be unrelated to 'unknown'.ts(2322)

Comment on lines 20 to 27
if (typeof (object as Iterable<T>)[Symbol.iterator] === "function") {
const iterable = object as Iterable<T>;
const iterator = iterable[Symbol.iterator]();
return iteratorPrototype.toIterable.call(iterator) as IterableIterator<T>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun question for you: why do we have to do iteratorFactory.call(object) and can't just do iteratorFactory()?

Suggested change
if (typeof (object as Iterable<T>)[Symbol.iterator] === "function") {
const iterable = object as Iterable<T>;
const iterator = iterable[Symbol.iterator]();
return iteratorPrototype.toIterable.call(iterator) as IterableIterator<T>;
}
const iteratorFactory = (object as Iterable<T>)[Symbol.iterator];
if (typeof iteratorFactory === "function") {
return toIterable.call(iteratorFactory.call(object));
}

Copy link
Contributor Author

@elimanzo elimanzo Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is because of the binding of this right, and we need to bind object to iteratorFactory when invoking the function?

Comment on lines 26 to 32
if (Object.prototype.isPrototypeOf.call(iteratorPrototype, object)) {
return iteratorPrototype.toIterable.call(
object as Iterator<T>,
) as IterableIterator<T>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (Object.prototype.isPrototypeOf.call(iteratorPrototype, object)) {
return iteratorPrototype.toIterable.call(
object as Iterator<T>,
) as IterableIterator<T>;
}
if (iteratorPrototype.isPrototypeOf(object)) {
return toIterable.call(object as Iterator<T>);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do this to suppress the linter

5c83bf9be54228f5f5663182b8b717d1

@elimanzo elimanzo requested a review from miorel September 21, 2024 20:37
@elimanzo elimanzo force-pushed the em-343-iterator-from branch from c82b09f to 7686304 Compare September 21, 2024 21:04
@elimanzo elimanzo merged commit 6bd0212 into main Sep 21, 2024
6 checks passed
@elimanzo elimanzo deleted the em-343-iterator-from branch September 21, 2024 21:10
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 this pull request may close these issues.

Add Iterator.from TypeScript goody

2 participants