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

Fix the very broken lodash flatten() type definitions #4791

Merged
merged 3 commits into from Jul 14, 2015

Conversation

pimterry
Copy link
Contributor

@pimterry pimterry commented Jul 1, 2015

The current lodash flatten type definitions:

  • Include method overloads that don't actually exist (in lodash or underscore or anywhere else I can find. My best guess is these were copied from _.map() and _.filter() accidentally somehow). You cannot pass a string to flatten() to pluck fields, or a callback to filter things, but we do currently have definitions for those overloads anyway

  • Have tests that specific the wrong expected types (along with a broken implementation to match):

    The test for _.flatten([1, [2], [3, [[4]]]]) expects a return type number[] (and gets it), but the actual return value of that expression is [1, 2, 3, [[4]]], which is not a number[].

  • Have overly specific types that exclude the real resulting type in most cases:

    _([1, 2, 3]).flatten() returns a lodash-wrapped [1, 2, 3], and has type LodashArrayWrapper<number> which is correct (i.e. it works correctly, if you call flatten on an already-flattened array).
    _([[1, 2, 3]]).flatten() returns a lodash-wrapped [1, 2, 3] with type LodashArrayWrapper<Array<number>> (incorrect, for the normal case).

This patch fixes those, conveniently fixing #2835 en route, although there are a couple of parts worthy of a little debate I think (see below). The changes are:

  • Type definitions for non-existent _.flatten() and _().flatten() methods overloads have been removed.

  • _(array).flatten() and _(array).flatten(bool) now both return any, rather than the overly specific (and usually wrong) type there before. Being any more specific than this is possible I think, but requires some heavy duty work around the generics for LoDashArrayWrapper everywhere to get to something like LoDashArrayWrapper<X, T extends RecursiveList<X>> { flatten(array: T): LoDashArrayWrapper<???, X> }. Not sure that's practical, so I've at least made it work for now.

  • _.flatten(array) now has the correct type and matching tests that actually track the real behaviour

  • _.flatten(array, bool) is now more accurate, but not always (although that's still an improvement on the current state). I'm not sure if this can be better, or whether it's worthwhile investigating that further.

    The issue is that the type of the result depends on the value of the boolean 2nd argument:

    _.flatten([[[1]]], true) recursively flattens, and returns [1]. This has type (number[][][]) => number[], or generally (RecursiveList<T>) => T. This is the type that I've given the method.

    _.flatten([[[1]]], false) meanwhile is equivalent to _.flatten([[[1]]]), and recurses only one level, returning [[1]], so here has a type of (number[][][]) => number[][]. This doesn't match the type definition. On the other hand, this is definitely the unusual case for this method (since the 'false' is unnecessary), and falling back to the version without the extra argument will give you the correct type.

    How concerned is DefinitelyTyped generally about things like this? Any thoughts on how this bit could be improved? Perfect world would be to use the correct return type depending on the runtime boolean value of the flag whenever that's known (likely 99+% of the time), but I think that's impossible. Option C is to just make it any-typed, but I'd rather not if at all possible.

@pimterry
Copy link
Contributor Author

pimterry commented Jul 1, 2015

Updated slightly: _.flatten(array, bool) now has a signature that is always correct, I think, by being a little more general. It now returns a union of a normal flattened list (for when the 2nd argument is true), or a recursive (i.e. partially flattened) list of, each containing whatever the common type of the given list argument was.

@vvakame
Copy link
Member

vvakame commented Jul 3, 2015

lodash/lodash.d.ts

to author(@bczengel). could you review this PR?
👍 or 👎?

check list

  • pass the Travic-CI test?

@bczengel
Copy link
Contributor

👍 This definitely looks better than it was. _.flatten has always been a tough method to define because of all of the possibilities. The addition of union types helps a lot.

vvakame added a commit that referenced this pull request Jul 14, 2015
Fix the very broken lodash flatten() type definitions
@vvakame vvakame merged commit ed4da20 into DefinitelyTyped:master Jul 14, 2015
@vvakame
Copy link
Member

vvakame commented Jul 14, 2015

@bczengel thanks!
@pimterry thanks mate!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants