tc39 / proposal-flatMap

proposal for flatten and flatMap on arrays
https://tc39.github.io/proposal-flatMap
214 stars 19 forks source link

Generalize flatMap to drop flatten #57

Closed gibson042 closed 6 years ago

gibson042 commented 6 years ago

Alternative to #56.

Adds an optional depth parameter to flatMap, specifies the necessary argument hockey, and abandons flatten because MooTools already homesteaded it.

rtablada commented 6 years ago

This PR seems to have 2 recommendations in one move.

Having a depth setting on flatmap becomes complex when the returned values are not pure arrays and leads to edgecases that I think could really be difficult to understand.

Take for example this data

let users = [
  {
    name: 'Ryan'
    posts: [ { id: 1, title: 'Learning Javascript', body: 'lorem' }]
  },
  {
    name: 'Tom'
    posts: [ 
      { id: 2, title: 'Mastering Javascript', body: 'lorem' },
      { id: 3, title: 'TC39', body: 'lorem' }
    ]
  }
]

Running users.flatMap(u => u.posts, { depth: 2 }) would be fine, but running with depth: 3 causes an error. This is a bit inconsistent and would be potentially hard to debug.

I would rather see flatMap left as is, and then add a recursiveFlatMap function.


This also doesn't completely solve a 1-1 replacement for flatten since it still would require a mapper function and could lead to potentially less perf since a native flatten could work with direct memory and references rather than running an iterator function.

If mapperFunction is present, then

This is seemingly skipped with flatten but is required for all flatMap operations even when the mapper function is a => a.

gibson042 commented 6 years ago

Running users.flatMap(u => u.posts, { depth: 2 }) would be fine, but running with depth: 3 causes an error.

How do you figure? users.flatMap(N, u => u.posts) under this PR is equivalent to users.map(u => u.posts).flatten(N) under the current master, which is [ {id: 1, …}, {id: 2, …}, {id: 3, …} ] for all positive N.

This also doesn't completely solve a 1-1 replacement for flatten since it still would require a mapper function

No, I made the map function optional. The whole point of this is to make flatMap a generalization of flatten so the latter can be omitted.

required for all flatMap operations

Good catch; fixed.

michaelficarra commented 6 years ago

Resolved by https://github.com/tc39/proposal-flatMap/commit/093eacc7fe0906e70f7626bf6c7d6e9dfc53cce9.