Skip to content

feat(es2018): add definitions for Array.prototype.flatten and Array.prototype.flatMap #20431

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

Merged
merged 3 commits into from
Jan 8, 2018

Conversation

benbraou
Copy link
Contributor

@benbraou benbraou commented Dec 4, 2017

This PR adds definitions for new array methods: Array.prototype.flatten and Array.prototype.flatMap.
The definitions are based (with modification) on https://github.com/tc39/proposal-flatMap/blob/master/polyfill.d.ts
The algorithm for flatten and flatMap is available here

Fixes #20410

@mhegazy
Copy link
Contributor

mhegazy commented Dec 4, 2017

Checked in with @bterlson about this feature, and seems like it is not guaranteed that this is going to make it in ES 2018 at the moment. We should know for sure by TC39 January meeting. For now, we should mark this as ESNext instead of ES2018.

@benbraou
Copy link
Contributor Author

benbraou commented Dec 5, 2017

@mhegazy thanks for update. Array.prototype.flatten and Array.prototype.flatMap are now marked as esnext

*
* @param depth The maximum recursion depth
*/
flatten<U>(this: U[], depth?: number): U[];
Copy link

@Jessidhia Jessidhia Dec 7, 2017

Choose a reason for hiding this comment

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

There is a reason why I wrote some of those overloads :)

With this definition, the result of [[1, 2], [3, 4]].flatten() is of type number[][] instead of number[].

The base case should look like:

flatten<U>(this: U[][], depth?: 1): U[];

There are other overloads for varying depths. However, if the depth is allowed to be an arbitrary number, then the return type is effectively any[].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kovensky thank you for the review! Indeed, I see the purpose of these overloads 😃. I have however two remarks:

  1. In the overloads you have defined, the "depth precision" stops at 7. Is this enough? Or too much ? I don't know what the guidelines are.
  2. With the definitions below
  flatten<U>(this: U[][][][][][][][], depth: 7): U[];
  flatten<U>(this: U[][][][][][][], depth: 6): U[];
  flatten<U>(this: U[][][][][][], depth: 5): U[];
  flatten<U>(this: U[][][][][], depth: 4): U[];
  flatten<U>(this: U[][][][], depth: 3): U[];
  flatten<U>(this: U[][][], depth: 2): U[];
  flatten<U>(this: U[][], depth?: 1): U[];
  flatten<U>(this: U[], depth: 0): U[];
  flatten<U>(depth?: number): U[];

if we have the following scenario:

const depthFourArray = [[[[4]]]];
const flatted = depthFourArray.flatten(4);

flatted is of type {}[] instead of number[] (because the last definition will be used). So, the developer will have to use type assertion.

Copy link

@Jessidhia Jessidhia Dec 8, 2017

Choose a reason for hiding this comment

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

Interesting... I would have expected the depthFourArray.flatten(4) to be flagged as the this of .flatten(4) is not a U[][][][][]. I think it's falling through to the last overload.

As for stopping at 7, it is an arbitrary stopping point, but I would highly question any use of .flatten that needs any depth higher than 2, maybe 3.

The last overload, flatten<U>(depth?: number): U[], is actually an any[] return in disguise, but will produce a stricter type ({}[]) if there is no context. I think it may be nicer to use than having to add an as assertion. In either way, we have no way of flagging the return as an "implicit any".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Back to working on this PR
I am going to add the overlaods. Thx!

flatMap<U, This = undefined> (
callback: (this: This, value: T, index: number, array: T[]) => U|U[],
thisArg?: This
): U[]
Copy link

@Jessidhia Jessidhia Dec 7, 2017

Choose a reason for hiding this comment

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

This will, unfortunately, do the wrong thing if U is deduced to be an Array, but I'm not sure if there's a way of doing <U extends anything but Array>. It's a place where the user will have to be careful.

Copy link

@Jessidhia Jessidhia Dec 7, 2017

Choose a reason for hiding this comment

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

Example:

const x = Array.from({ length: 2 }).flatMap((_, i) => (
  // U is number[]
  i === 0 ? [1] : [[2]]
))
// x is number[][] but should actually be Array<number|number[]>

Copy link
Contributor Author

@benbraou benbraou Dec 8, 2017

Choose a reason for hiding this comment

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

@Kovensky Indeed. However, I don't know either how we can achieve <U extends anything by Array>.
Meanwhile, we can change the definition to
flatMap<U, This = undefined> ( callback: (this: This, value: T, index: number, array: T[]) => U ,thisArg?: This ): U[];
I will be available to continue working on this PR starting from December 15th. Thanks for the feedback!

Choose a reason for hiding this comment

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

It's better if the callback's return is U[] instead of U, otherwise this looks no different, type-wise, from a regular .map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kovensky according to #20724, using conditional types it will be possible to define a type that extends anything but array.
With something like this I guess

type Diff<T, U> = T extends U ? never : T;
type NonArray<T> = Diff<T, Array>;

flatMap<U: NonArray, This = undefined> (
        callback: (this: This, value: T, index: number, array: T[]) => U|U[],
        thisArg?: This
): U[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kovensky for now, I will update the callback's return as U[] as you suggested

@mhegazy mhegazy closed this Jan 8, 2018
@mhegazy mhegazy reopened this Jan 8, 2018
@mhegazy mhegazy closed this Jan 8, 2018
@mhegazy mhegazy reopened this Jan 8, 2018
@mhegazy mhegazy merged commit 804eb32 into microsoft:master Jan 8, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants