-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arr: Array<T>, | |
arr: Iterable<T>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function argument arr
accepts Array<T>
here as we are accessing the .length
property, which doesn't exist on normal Iterator
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Adds proper types for Array helper methods. Additional comments are provided as inline comments.