[@types/lodash] _.filter types break when combined with _.get

[andnp]

I think that we are somehow getting into the wrong function overload for _.filter here.

const obj = {
    hi: false,
    there: true
};
const merp: string[] = _.filter(_.keys(obj), (x) => {
    return _.get(obj, x);
});

is a simplified version of what we are doing.

const obj = {
    hi: false,
    there: true
};
const merp: string[] = _.filter(['hi', 'there'], (x) => {
    return _.get(obj, x);
});

also breaks in the same way (although I think it actually prefers yet another overload).

In the first example the filter function is inferred to be:

(x: string) => somethingCrazy

in the second:

(x: somethingCrazy) => somethingCrazier

It seems this broke after #20795 went in (which has some changes to the _.filter types).

For now we can fix it with:

const merp: string[] = _.filter(['hi', 'there'], (x) => {
    return !!_.get(obj, x);
});

[andnp]

Tagging some authors here:
@thorn0 @DanielRosenwasser @aj-r @RyanCavanaugh

[thorn0]

TS version? Compiler settings?

[andnp]

TS version 2.5.3 and 2.6.1 and I'm not sure the compiler settings are relevant in this case?
In any case:

{
  "compilerOptions": {
    "baseUrl": "./src",
    "outDir": "./dist/",
    "sourceMap": true,
    "strict": true,
    "allowJs": true,
    "noEmitOnError": true,
    "module": "commonjs",
    "target": "es5",
    "lib": [
      "dom",
      "es2015",
      "es2016"
    ]
  }
}

[thorn0]

It thinks that (x) => { return _.get(obj, x); } is PartialDeep<string[][keyof string[]]>. which is effectively a union of the types of all the methods of string[].

[andnp]

Well that's not good..

I can't figure out where it would be picking up PartialDeep from the _.get typings though. None of the return signatures in the _.get overloads return a PartialDeep anything.

[thorn0]

It's a problem of the _.filter typings only. We can replace the _.get call with any other expression of type any, and the problem will still be there. It looks really complicated, and I still can't think of a solution for this issue. If only we had some way to express types like "any object but not a function".

[aj-r]

This is intended. The callback for _.filter is expected to return a boolean type. If _.get does not return boolean, then it won't work. In the example you've given, it won't return boolean because x has type string. You could try something like this (note the Array<keyof typeof obj>):

const obj = {
    hi: false,
    there: true
};
const merp: string[] = _.filter(_.keys(obj as Array<keyof typeof obj>), (x) => { // or `Array<"hi" | "there">
    return _.get(obj, x);
});

or alternatively:

const obj: { [key:string]: boolean } = {
    hi: false,
    there: true
};
const merp: string[] = _.filter(_.keys(obj), (x) => {
    return _.get(obj, x);
});

However this example seems a bit contrived, and I wonder why you don't just use obj[x] instead of _.get(obj, x). It might be helpful to provide more detail of what you're actually trying to do.

If your _.get function really does not return a boolean (which seems to be the case), then the intended solution is to use return !!_.get(obj, x); like you mentioned in the description.

[andnp]

The docs for _.filter say that the predicate function is expected to return a truthy value, not necessarily a boolean, which is what we were leveraging here.

Unfortunately, this example may seem contrived since it is a minimal and reproducible example and not my full production code. I suppose to be a bit more true to how it is being used in production you could imagine this situation:

const obj = *some very large and deep object with differing properties and keys at the leaves*;
const merp: string[] = _.filter(_.keys(obj), (x) => {
    // some logic to figure out what leaf to access
    const x = ['result', 'path', 'from', 'previous', 'logic']
    return _.get(obj, x);
});

Anyways, this was working a few days ago as expected because _.get should return some value or undefined which is truthy or falsy (but not necessarily true or false) as expected by the _.filter predicate function according to the docs.

[andnp]

Actually, with both of your help it is more clear to me what is happening; so a much more minimal example would be:

const merp: string[] = _.filter(["a ", "   ", "c"], (x) => x.trim());

again, maybe slightly contrived, but it's not hard to imagine a place where filter could be used to return a non-boolean result.

[aj-r]

There was a lengthy discussion here about whether callbacks should return boolean (true/false) or any (truthy/falsy). The final conclusion was that they should only accept boolean as a return type. If you have a truthy/falsy value, you can easily convert it to true/false using !!.

That said, the Array.filter definition in lib.es6.d.ts has a callback that accepts any as a return type. However, other functions with callbacks (some, every) only accept boolean as a return type. I think this is a bug in lib.es6.d.ts - all of the callbacks should accept a consistent return type (either boolean or any).

Mentioning @Andy-MS because he was involved in this discussion.

[andnp]

Ah, thanks for the link! I missed this as I was skimming that PR. Still not used to github folding some of the comments in "outdated" tags, so I keep missing some of these discussions.

This is an interesting discussion that I hadn't considered: Typescript seems unique among typed languages in that it even has the concept of "truthy" and "falsy". Typescript, however, is also unique in its ability to duck-type, of which truthy/falsy seems to be a derivative case. Upon further consideration, perhaps this is not so true.

My immediate reaction here is that leveraging truthy/falsy is an incredibly common pattern in javascript that Typescript may not be capable of supporting. I'd almost be interested in defining two types:

type Truthy = true | string | number | object | function;
type Falsy = false | '' | 0 | null | undefined;

This, however, would not be possible right now as there is no way of distinguishing string and '', as well as number and 0. Not to mention that my example above would still break even in this pattern as there is no way to know at compile time that ' '.trim() would be falsy. Requiring that the predicate returns a boolean would mean significantly better type safety. Allowing the predicate to return any would mean that (x) => { x.trim() } is a "valid" predicate as @thorn0 pointed out.

The flipside here is that enforcing a boolean is slightly divergent from the lodash docs, which meant finding the source of the error for me was not immediately obvious. Additionally, this is code that has been building/running successfully for months that suddenly broke after a patch version bump in the npm repository.

---

TL;DR, after my wordy "thinking aloud": I am indifferent to whether or not the predicate should return a boolean (though I wish the errors were more helpful other than string[] doesn't match some-big-glob-of-types-that-are-the-methods-of-string, but that's likely not possible/easy to fix).
I would rather not have to lock down my package.json to not automatically grab patch versions though. Things, like adding overloads to functions, or fixing clear bugs that are divergent from the documentation are patches to me, but enforcing an undocumented community-driven pattern feels more like a minor version bump. But I'm not sure what DefinitelyTyped's standards for semantic versioning are.

Anyways, thanks for the discussion. This makes a lot more sense to me, now seeing the reason behind the change.

[ghost]

See also microsoft/TypeScript#20033

[Retsam]

(Full disclosure: I'm a teammate of @andnp , looking at the same error in the same codebase and wanted to add my thoughts: I'm not trying to "sock-puppet" here)

I appreciate the efforts to improve lodash typings, but I disagree with the changes to _.filter on three levels:

On the first level, I don't really agree with the idea of changing _.filter to only accept booleans in the first place: it goes against both lodash's docs and the wider JS convention that truthy values are always (or at least virtually always) accepted in replace of booleans.

IMO, the typings for a library should reflect the conventions and the documented behavior of the library as much as possible, and generally shouldn't add opinion-based restrictions that don't exist in the original library.

For one thing, it makes migration to TS harder when there are these sort of restrictions in TS that don't exist in the JS versions. Yeah, individually it's an easy fix (add !!), but it can become burdensome quickly when migrating existing code: especially if there's a lot of these opinionated restrictions. It can quickly get into a "death by a thousand cuts" sort of scenario.

---

On the second level, (i.e. ignoring the previous objections) I don't think the end result of the current implementation of that restriction is good. If we want to restrict filter callbacks to only return booleans, then _.filter should raise a type error when passed a function that doesn't return a boolean. The current behavior - where it accepts the function but infers a garbage return type¹ - is particularly worrisome, as it makes it tricky to find the source of the error.

When debugging this it wasn't even immediately obvious that _.filter is the issue, (since that's not where the error will be raised), and once the issue is tracked down to _.filter it's not obvious that the issue is the return type.

Plus, it just ends up being inconsistent: it's unintuitive that _.filter(array, 'foo') is accepted by the typings, but _.filter(array, x => x.foo) isn't, (if x.foo isn't boolean), since the former is meant to be a shorthand for the latter.

And, additionally, this breaks if the function passed to _.filter returns any, which is unfortunate, too, as I think typings should always handle any as cleanly and as permissively as possible.

---

And on the third level, as andyp said, it's painful that this happened in a patch version. I'm not sure if it's just an endemic problem within all of DefinitelyTyped, but breaking type changes on patch version isn't fun.

I don't know that much about the DefinitelyTyped repository's mechanics, but if there's a way to avoid that in the future, it'd be appreciated.

---

So, personally, I'd really like to see some changes to _.filter. Either just return it to match the lodash documentation of accepting "truthy" predicates, or at least bump the minor version and find a way to make this change in a more ergonomic way.

---

¹ It really is an ugly type, after doing const xs = _.filter([] as any[], x => x.x); the type of xs is inferred to be:

(number | ((searchElement: any, fromIndex?: number | undefined) => boolean) | (() => string) | ((...items: any[]) => number) | (() => any) | {
    (...items: ReadonlyArray<any>[]): any[];
    (...items: any[]): any[];
} | ((separator?: string | undefined) => string) | (() => any[]) | ((start?: number | undefined, end?: number | undefined) => any[]) | ((compareFn?: ((a: any, b: any) => number) | undefined) => any[]) | {
    (start: number, deleteCount?: number | undefined): any[];
    (start: number, deleteCount: number, ...items: any[]): any[];
} | ((searchElement: any, fromIndex?: number | undefined) => number) | ((callbackfn: (value: any, index: number, array: any[]) => boolean, thisArg?: any) => boolean) | ((callbackfn: (value: any, index: number, array: any[]) => void, thisArg?: any) => void) | (<U>(callbackfn: (value: any, index: number, array: any[]) => U, thisArg?: any) => U[]) | {
    <S extends any>(callbackfn: (value: any, index: number, array: any[]) => value is S, thisArg?: any): S[];
    (callbackfn: (value: any, index: number, array: any[]) => any, thisArg?: any): any[];
} | {
    (callbackfn: (previousValue: any, currentValue: any, currentIndex: number, array: any[]) => any): any;
    (callbackfn: (previousValue: any, currentValue: any, currentIndex: number, array: any[]) => any, initialValue: any): any;
    <U>(callbackfn: (previousValue: U, currentValue: any, currentIndex: number, array: any[]) => U, initialValue: U): U;
} | (() => IterableIterator<[number, any]>) | (() => IterableIterator<number>) | (() => IterableIterator<any>) | {
    <S extends any>(predicate: (this: void, value: any, index: number, obj: any[]) => value is S, thisArg?: any): S | undefined;
    (predicate: (value: any, index: number, obj: any[]) => boolean, thisArg?: any): any;
} | ((predicate: (value: any, index: number, obj: any[]) => boolean, thisArg?: any) => number) | ((value: any, start?: number | undefined, end?: number | undefined) => any[]) | ((target: number, start: number, end?: number | undefined) => any[]))[]

[ghost]

@Retsam what would you think about an option to allow passing of potentially-truthy-potentially-falsy values to boolean? There are probably a million uses of boolean already in existing declarations, and it would lose information to replace all of those with any even if that's technically allowed. See microsoft/TypeScript#20033.

[Retsam]

@Andy-MS That'd definitely address my main objection to the change (i.e. the "first level" objection from my previous post), on principle. It would probably be the best of both worlds to allow the typings to be stricter and allow users to opt-out of that strictness, if they prefer.

But I'm not sure I'd want lodash to be strict in the interim while that Typescript feature doesn't exist,
and in any case, I still think the "ergonomics" of the current implementation aren't great. I'm not sure the proposed Typescript option would even have any affect on the current state of _.filter, since it's not failing because boolean isn't any, it's failing because _.filter is spitting out junk types.

[aj-r]

I'm inclined to agree. We could make these callbacks return lodash's `NotVoid ` type, which is just `{} | null | undefined` until typescript has a looser version of a Boolean return type. That's better than `any` because we at least force people to return a value in the callbacks. On Nov 21, 2017 5:19 PM, "Retsam" <[email protected]> wrote: @Andy-MS <https://github.com/andy-ms> That'd definitely address my main objection to the change (i.e. the "first level" objection from my previous post), on principle. It would probably be the best of both worlds to allow the typings to be stricter and allow users to opt-out of that strictness, if they prefer. But I'm not sure I'd want lodash to be strict in the interim while that Typescript feature doesn't exist, and in any case, I still think the "ergonomics" of the current implementation aren't great. I'm not sure the proposed Typescript option would even have any affect on the current state of _.filter, since it's not failing because boolean isn't any, it's failing because _.filter is spitting out junk types. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21485 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpeswEgMxob6sCRwZF6h9hIdAQq80R4ks5s40xigaJpZM4Qb90F> .

[logan-dunbar]

I'm finding the same issue with _.reject coupled with _.isUndefined, wondering if it is related:

const array = ['some', 'value', undefined, 'another'];
_(array).reject(_.isUndefined).value(); // any[]
_(array).reject(a => _.isUndefined(a)).value(); // string[]

I can type the reject function directly, which types the return as expected, but I feel like there should be a way to infer this typing, as in the second example above:

_(array).reject<string>(_.isUndefined).value(); // string[]

TS 2.9.2 with default compiler options.

[orta]

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.