Skip to content
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

feat: add flat/flatMap #6948

Closed
wants to merge 1 commit into from

Conversation

keithamus
Copy link
Contributor

Just taking a punt on types for flat and flatMap. These are part of a TC39 Stage 4 proposal that has been implemented in all browsers.

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Oct 8, 2018

It's not standardized yet and possibly flat will be unshipped due to web compatibility issues
https://bugs.chromium.org/p/chromium/issues/detail?id=888128#c15

@nmote nmote added the Library definitions Issues or pull requests about core library definitions label Jan 3, 2019
@nmote
Copy link
Contributor

nmote commented Jan 25, 2019

It looks like they got over the concern and this is shipped in Firefox and Chrome, at least. Since it's shipped in browsers, and the proposal is pretty far along, I'm comfortable adding support for this.

Unfortunately I'm not sure this is possible to accurately type in Flow. For example, with the proposed libdef:

const x: Array<Array<number>> = [[1]];
const y: Array<Array<number>> = x.flat();
y[0]; // This will actually be `number`, but Flow will tell you it is `Array<number>`

Basically, we would want a type system feature which would allow us to say "You can call this if the this type is Array<Array<T>>, in which case it will return Array<T>." Flow has no such capability, so I don't think we can type this very precisely.

As an aside, Skip lets you do this and it's really cool.

I think that updating this to use Array<mixed> where necessary would at least let us merge this PR. It's not very precise, but it should at least be typesafe.

@nmote nmote self-assigned this Jan 25, 2019
@wchargin
Copy link
Contributor

drive-by thought: can you do anything with

type Join<T> = Array<$ElementType<$ElementType<T, number>, number>>;

?

@lydell
Copy link

lydell commented Feb 10, 2019

Would it be possible to add only flatMap for now, and flat later when it is been figured out how to do so? I regularly miss flatMap in my code, but not flat.

@chicoxyzzy
Copy link
Contributor

flat is landed to ES2019 so it's safe now to add it

@nmote
Copy link
Contributor

nmote commented Apr 11, 2019

I'll merge this once my concerns described above are addressed. I suspect that the best we can do is Array<mixed> for flat. I have similar concerns around flatMap.

@goodmind
Copy link
Contributor

goodmind commented Apr 11, 2019

@nmote React.ChildrenArray does flattening

// A children array can be a single value...
const children: React.ChildrenArray<number> = 42;
// ...or an arbitrarily nested array.
const children: React.ChildrenArray<number> = [[1, 2], 3, [4, 5]];
// Using the `React.Children` API can flatten the array.
const array: Array<number> = React.Children.toArray(children);

@FezVrasta
Copy link
Contributor

Could someone from the Flow team take a look at this PR? Not having support for standard JS features is pretty bad...

Copy link
Contributor

@nmote nmote left a comment

Choose a reason for hiding this comment

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

My earlier comment still stands. This is not typesafe as written. Requesting changes to make this more explicit.

Others are welcome to open competing PRs if they do not want to wait for @keithamus to respond.

@goodmind
Copy link
Contributor

flatMap can be accepted, only flat is difficult one, but this PR needs tests

@goodmind
Copy link
Contributor

#7854

@eilvelia
Copy link

eilvelia commented Jun 28, 2019

Here is an accurate Array#flat implementation in TypeScript via recursion.

type Dec = [-1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20]
type Flat<A, D extends number> = {
    '1': A,
    '0': A extends ReadonlyArray<infer T> ? Flat<T, Dec[D]> : A
}[D extends -1 ? '1' : '0']
declare function flat<A, D extends number = 1>(array: A, depth?: D): Flat<A, D>[]

If someone knows how to rewrite it in Flow, feel free to propose ideas

@goodmind goodmind added the Superseded PRs that solve the same issue as a "superseding" PR that already landed label Jul 27, 2019
@nmote
Copy link
Contributor

nmote commented Oct 25, 2019

I've merged #7854 which added a declaration for flatMap, but my concerns around flat still stand. I'd consider a PR which added flat with a return type of Array<mixed>. That is not likely to be especially useful, but I don't think we can currently type it more precisely.

@nmote nmote closed this Oct 25, 2019
@nnmrts
Copy link
Contributor

nnmrts commented Dec 16, 2019

@nmote

Another great example of "uhm yeah this is a standard js feature but you just can't use it and you basically have to decide between using flow or not"

At least let me freaking disable certain parts of flowlib, or let me extend it locally, or whatever.

The way you guys handle these kind of "don't think we can type it currently more precisely" issues is mindbogglingly frustrating. Then don't type it and just ignore it. Or use any or mixed OR ANYTHING. Or idk, add an unsafe mode. Or a flowconfig option for specific types.

It's not like flow is unusable, no, flow actually makes 20% of javascript unusable.

Correct me if I'm wrong, but my only options in these cases would be:

  1. downloading an extra library like lodash and using their untyped functions (suddenly flow has no problem with that (?) (except that coverage will go down))
  2. refactoring all instances of a unusable standard feature like flat in with some other, less efficient and more verbose method like flatMap (or spread with Object.assign, or Array.reduce with Object.entries, etc.)
  3. enabling no_flowlib in my flow config, copying the flowlib locally and changing or adding definitions after learning how that even works because writing library definitions isn't really easy and when I'm finished with that, flow probably got a new minor patch update that's useful that I can only use if I start this whole process all over again
  4. sprinkle dozens of $FlowFixMe comments over my codebase 🆗🆒
  5. enabling suppress_type and somehow adding it to my code 😮💯
  6. just ignoring all these funny wavy red underlines in my code 🧠
  7. not using flow at all 🤯
  8. adding a PR 🙈

@nmote
Copy link
Contributor

nmote commented Dec 16, 2019

You've correctly identified your options: either work around this missing libdef or submit a PR that addresses the issues I've raised here. If you do so, feel free to cc me and I'll make sure to take a look at it quickly.

nnmrts added a commit to nnmrts/flow that referenced this pull request Dec 17, 2019
facebook-github-bot pushed a commit that referenced this pull request Dec 19, 2019
Summary:
ref: #6948

according to https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/flat
Pull Request resolved: #8237

Reviewed By: dsainati1

Differential Revision: D19156287

Pulled By: nmote

fbshipit-source-id: dcbb31b8a3e64f7c4675165197e3d85013cac8ce
facebook-github-bot pushed a commit that referenced this pull request Apr 28, 2020
Summary:
Currently, `.flat()` always returns `Array<mixed>` which limits its usefullness. While I don't think we can type it perfectly in the general case, I believe I have come up with something that will improve support for the common case: calling `xs.flat()` where `xs` is an arry of arrays, and no depth argument is supplied to `.flat()`. We still don't support depth arguments (unless they are the literal number `0` or `1`), and don't support calling `.flat` on an array like `[1, 2, [3, 4]]`, but at least this common pattern is supported.

Related: discussion in #6948 on GitHub (#6948)

Reviewed By: nmote

Differential Revision: D21168223

fbshipit-source-id: cd74d9b0e87ce48fa67211f23a38b17c3069e276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Library definitions Issues or pull requests about core library definitions Superseded PRs that solve the same issue as a "superseding" PR that already landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants