Allow a specified depth for flatten

[paulfalgout]

Per a new TC39 proposal http://bterlson.github.io/proposal-flatMap/#sec-Array.prototype.flatten they're suggesting a depth argument.

This was fairly trivial to implement without breaking the current functionality.

Thoughts?

[akre54]

Awesome. 👍

[megawac]

👍

[jdalton]

Should a depth of 0 be seen as Infinity?

[paulfalgout]

so you think we need a if (depth === 0) return array; ?

[akre54]

👍

[jdalton]

Should return a shallow clone/slice which should shake out naturally if 0 is allowed to be used as a depth (no need for a if or special branch)

[paulfalgout]

so then if (!depth && depth !== 0) return

[jdalton]

@paulfalgout To preserve as much back compat ya I think that would do.

[paulfalgout]

So looking into both 0 and negative values, it seems much cleaner to handle this in _.flatten than it would be to try to get the internal flatten to play nice with these additional scenarios, particularly related to the "strict" argument. I also assumed that negative would act the same as 0?

[jdalton]

it seems much cleaner to handle this in _.flatten than it would be to try to get the internal flatten to play nice with these additional scenarios

Oh cool!

I also assumed that negative would act the same as 0

Yeah.

[michaelficarra]

Might want to ToNumber depth here so we're not passing true to flatten.

[paulfalgout]

we're internally calling flatten with booleans, so I don't know if we want to enforce that at the moment?

[megawac]

can we updat ethe internal calls to use a number

[michaelficarra]

LGTM 👍

[jridgewell]

This conditional confuses the hell out of me. Maybe:

if (depth === false) {
  depth = Infinity;
} else if (depth === true) {
  depth = 1;
} else if (depth < 0) {
  depth = 0;
}

[paulfalgout]

the internal flatten doesn't handle depth 0 at all and will conflict with whatever "strict" is doing.
depth as with shallow is optional so the default value is Infinity, but to keep with the current API false should also be assumed as Infinity so I suppose it could be:

if(depth === false || _.isUndefined(depth)) {
  depth = Infinity;
} else if (depth <= 0) {
  return _.clone(array);
}

We can certainly add the else if (depth === true) { but flatten is called internally with true elsewhere.

[paulfalgout]

This was initially easy, but got complicated with edge cases and without refactoring internals a bit.
Any ideas what we should do with this PR? I've answered the two outstanding questions, but I'm not certain they're resolved.

[jgonggrijp]

@paulfalgout I think you already did the right thing. I think this is not even a breaking change. I'm in favor of merging this, but it has gotten a bit out of sync and I just opened #2849 which will make this even more out of sync. So let's postpone merging this just a little bit more.

Really good job!

[jgonggrijp]

@paulfalgout I merged #2849 three weeks ago and I nearly forgot to get back to you. Sorry about that.

So master should be stable again for a while and I see no reason not to finish your PR. With some luck, we should be able to include it in the upcoming 1.11 release. In order to do so, we need to update your patch first. Please let me know whether you'd like to do this yourself, or whether you'd prefer me to take care of these final steps:

1. Check out the feature branch and pull in the latest changes from master. Disregard the merge conflict for now and postpone the merge commit until step 5.
2. Run a fresh npm install to update all the tools and install our commit hooks. These will take care of updating the underscore.js.
3. Update modules/_flatten.js and modules/flatten.js (note the underscore) with the changes that you originally made in the old underscore.js.
4. Run git add underscore.js to silence the warnings about the merge conflict. The conflict markers will be overwritten by the pre-commit hook.
5. Commit and push.

[jgonggrijp]

I'm now looking into merging this by executing the above steps myself. On closer inspection, I noticed that many internal calls to flatten use the internal flatten with a boolean. I think I'll address this by moving the guard that changes false into Infinity from the public to the internal flatten.

[paulfalgout]

@jgonggrijp thanks for hopping on this.. I've been a little out of it as of late.

[jgonggrijp]

Welcome.