support type predicate callback functions on Array<T>.filter #11858
Conversation
|
Hi @sebek64, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
@sebek64, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
Looks correct to me |
|
Shouldn't this go to |
|
Yes, I haven't read the contributing instructions with the required attention. I'll provide a fix. |
|
I've pushed an updated version. I'm not an expert on typescript internals. I've 'baseline-accept'ed the changes, but I don't know if that is correct. I've found that ReadonlyArray already has these overloads both in lib/ and src/lib. So this commit adds a similar overloads to Array. |
lib/lib.d.ts
Outdated
| @@ -1258,6 +1258,12 @@ interface Array<T> { | |||
| * @param callbackfn A function that accepts up to three arguments. The filter method calls the callbackfn function one time for each element in the array. | |||
| * @param thisArg An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value. | |||
| */ | |||
| filter<S extends T>(callbackfn: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[]; | |||
There was a problem hiding this comment.
The files under lib are not edited manually, they are generated as part of the build. please remove them. you only need to update src\lib
There was a problem hiding this comment.
Done, removed all changes of lib/* files.
lib/lib.es5.d.ts
Outdated
| @@ -1258,6 +1258,12 @@ interface Array<T> { | |||
| * @param callbackfn A function that accepts up to three arguments. The filter method calls the callbackfn function one time for each element in the array. | |||
| * @param thisArg An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value. | |||
| */ | |||
| filter<S extends T>(callbackfn: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[]; | |||
There was a problem hiding this comment.
Please change the other filter methods on ReadOnlyArray and the TypedArray variants e.g. Int32Array.
There was a problem hiding this comment.
As for ReadonyArray, this method is already there. For typed arrays, it doesn't make sense to me (since there is no type inheritance like for generic arrays).
|
This pull request seem to be a duplicate of #10916 |
|
Yes, that's true. I haven't noticed it when I created this one. So please close one of them and merge the other one. |
- this is already supported on ReadonlyArray<T>
|
I've rebased the branch to current master. Is there any possibility that this gets merged before it runs out of sync again? |
| * @param callbackfn A function that accepts up to three arguments. The filter method calls the callbackfn function one time for each element in the array. | ||
| * @param thisArg An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value. | ||
| */ | ||
| filter<S extends T>(callbackfn: (this: void, value: T, index: number, array: T[]) => value is S): S[]; |
There was a problem hiding this comment.
ReadonlyArray and Array should stay in sync. so please update both.
There was a problem hiding this comment.
Also consider coalescing the overloads using type argument defaults:
filter<S extends T, Z = undefined>(callbackfn: (this: Z, value: T, index: number, array: T[]) => value is S, thisArg?: Z): S[];There was a problem hiding this comment.
Ad first: ReadonlyArray is already updated (see commit comment), so this commit actually brings both classes in sync.
Ad second: I'm not sure if this is the same, as Z is void in the first case and undefined in second. And tt is used the same way 5 lines below the change.
|
This issue has been fixed by #16223 and can be closed. You can try it out in |
This overload of Array.filter function seems useful, but I'm not sure if it doesn't break something.