[helpers ts conversion] Arrays #16518
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/57008 |
packages/babel-helpers/src/helpers/iterableToArrayLimitLoose.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Nicolò Ribaudo <hello@nicr.dev>
Co-authored-by: Nicolò Ribaudo <hello@nicr.dev>
|
There are some tests failing from the recent suggestions/changes. I'll get those fixed! |
|
Oh well, it looks like one of my suggestions was wrong if it changes tests behavior 😅 |
@nicolo-ribaudo Yeah I thought it was only an invalid snapshot but it looks like actual tests were failing. I've reverted. Looks like everything is green now. |
| /* @minVersion 7.0.0-beta.0 */ | ||
|
|
||
| export default function _iterableToArrayLimitLoose<T>( | ||
| arr: Array<T>, |
There was a problem hiding this comment.
| arr: Array<T>, | |
| arr: Iterable<T>, |
There was a problem hiding this comment.
Ahh, right. Now I recall why I did this. It's because we are accessing the .length property below on arr, which only exists on Array. See: #16518 (comment)
Co-authored-by: Sukka <isukkaw@gmail.com>
…tLoose.ts" This reverts commit dc93ebc.
| var _arr: T[] = []; | ||
| var step; | ||
| iterator = iterator.call(arr); | ||
| while (arr.length < i && !(step = iterator.next()).done) { |
There was a problem hiding this comment.
The function argument arr accepts Array<T> here as we are accessing the .length property, which doesn't exist on normal Iterators.
There was a problem hiding this comment.
I suppose this would include String as well. I'll update the arr arg to accept Array<T> | String
There was a problem hiding this comment.
Ughhh I'm quite confident that this is a bug, and we should be using _arr instead of arr. Can you mark arr as Iterable<T>, and add a @ts-expect-error here? I'll open an issue to track it.
There was a problem hiding this comment.
Oh whaat, this helper (and slicedToArrayLoose) have never been used. They have been introduced in ae7d536, but there has never been any code actually loading them.
It doesn't matter how they are converted to TS at this point -- could you instead open a separate PR to delete them? 😅
There was a problem hiding this comment.
iterableToArrayLimitLoose.ts needs to be removed in a separate PR. Its types are a bit wrong, but so is them implementation so it's fine.
Adds proper types for Array helper methods. Additional comments are provided as inline comments.