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

Array.prototype.filter definition forces a boolean #5850

Closed
denis-sokolov opened this issue Dec 1, 2015 · 15 comments
Closed

Array.prototype.filter definition forces a boolean #5850

denis-sokolov opened this issue Dec 1, 2015 · 15 comments
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@denis-sokolov
Copy link

It is natural to use Array.prototype.filter to filter items with missing information:

type Foo = { prop: string }

const a: Foo[] = [
    { prop: 'bar' },
    { prop: null },
    { prop: 'baz' }
] 

a.filter(x => x.prop)

Unfortunately, this does not compile, as filter definition requires the result of the filter function to be strictly boolean. Would it make sense to make the filter definition less strict?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

how about:

a.filter(x => !! x.prop)

Alternatively, you can define a new signature for filter in a file in your project as such:

interface Array<T> {
    filter(callbackfn: (value: T, index: number, array: T[]) => any, thisArg?: any): T[];
}

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Dec 1, 2015
@denis-sokolov
Copy link
Author

how about a.filter(x => !! x.prop)

Well, yes, type casting is the current way to appease the compiler.
My point was that we probably would want to not need to appease the compiler.

you can define a new signature for filter in a file in your project

Sure, thanks for the suggestion. I know how to do that.

My point was not how to workaround this limitation in my project, I have no problem with that.
My point was to suggest an improvement to the core for the benefit of everybody.

@DanielRosenwasser DanielRosenwasser added Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Suggestion An idea for TypeScript and removed Question An issue which isn't directly actionable in code labels Dec 1, 2015
@DanielRosenwasser
Copy link
Member

We've run into this sort of problem with lib.d.ts a few times now (e.g. #4002). JavaScript is technically okay with a lot of things, but basically it comes down to trying to help users catch mistakes when we can. Forgetting to perform some condition is an easy mistake, and !! is such an easy remedy for when you want to say "It's okay, I know what I'm doing."

If we did ever want to fix this, we couldn't just change the return type to any, because users could accidentally forget to return something in their callback. A more appropriate return type would be the (fairly awkward) boolean | string | number | symbol | Object.

@denis-sokolov
Copy link
Author

@DanielRosenwasser, you’re right, this is a judgement call about inconvenience vs catching errors.
It is up to you, of course, to make the call.

Do consider, that in my point of view, .filter is equivalent to an if statement. And surely you do not plan to enforce mandatory type conversion in an if condition? But the same could be said about that: ‘Forgetting to perform some condition is an easy mistake (if (foo) {}), and !! is such an easy remedy (if (!!foo) {}).’

@RyanCavanaugh RyanCavanaugh added the In Discussion Not yet reached consensus label Dec 1, 2015
@RyanCavanaugh
Copy link
Member

I'm with @denis-sokolov on this one. We shouldn't pretend if (arbitrary_expr) is safe for if and dangerous for filter.

@adidahiya
Copy link
Contributor

+1, it is inconsistent to have different levels of strictness for if and filter. I'd love to implement a TSLint rule to flag implicit coercion for users who want the additional safety though: palantir/tslint#625

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Good First Issue Well scoped, documented and has the green light Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript and removed Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript In Discussion Not yet reached consensus labels Jan 5, 2016
@RyanCavanaugh
Copy link
Member

Approved. @mhegazy said there's no other suspicious use of boolean in lib.d.ts but someone should verify (Array#some ?).

Return type of the callback should be any

@DanielRosenwasser
Copy link
Member

Why should the return type be any if you want to catch errors where users forget to return a value?

@RyanCavanaugh
Copy link
Member

I was arguing for {}; @ahejlsberg argued that not returning a value is a valid always-false filter (I hope that's not a total strawman representation?)

@RyanCavanaugh
Copy link
Member

Oh, and any is consistent with this behavior in terms of non-returning functions:

function fn() { }
if (fn()) { // <-- not an error
}

@DanielRosenwasser
Copy link
Member

Why would you ever do that instead of using forEach? Anyone writing that is clearly making a mistake or doesn't know better.

@Elephant-Vessel
Copy link

I use typescript because I like static typing, I'm not sure I like this. If this is ok, who'd say that no more ugly type coersions in javascript won't diffuse into typescript "because we can do it in javascript"?

I'd counter-propose a breaking change for the if and require an explicit boolean there instead. In the end, typescript is all about adding the layer of typing that JS is missing right?.

@mjohnsonengr
Copy link

@Elephant-Vessel I'd agree with you were this some other kind of type coersion, but boolean coersion in the form of truthy and falsey values has become a staple of the language for many TypeScript developers, and doesn't detract from the purpose of the language at all.

Your breaking change proposal would break such a large portion of TypeScript codebases out there. Would you also propose breaking changes for && and || so I can't do this:

var propOrDefault = someThing && someThing.itsProp || defaultProp;

@Elephant-Vessel
Copy link

A coherent conceptual model of a language is more important for me than some neat shorthands - So yes I do, that should at least return either true or defaultProp. And preferably complain about the implicit conversions to booleans.

Of course I was rhetorical about actually making this a breaking change, but it could at least be a compiler setting for us that don't want these compromises.

Edit: Oh and by the way, that stuff is in my opinion better handled by giving us a decent set of operators:

var propOrDefault = someThing?.itsProp ?: defaultProp (Elvis preferably only checking for null if non-boolean type of itsProp or truthiness (and null?) if boolean type)

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Apr 18, 2016
@mhegazy mhegazy added this to the TypeScript 2.0 milestone Apr 18, 2016
@mhegazy mhegazy closed this as completed Apr 18, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Apr 18, 2016

Fixed in #7779

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants