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

[TS] `filter(Boolean)` loses type information, TypeScript 3.5 makes it worse #4959

Closed
rkirov opened this issue Aug 9, 2019 · 11 comments · Fixed by #4961

Comments

@rkirov
Copy link

commented Aug 9, 2019

Bug Report

More of a heads up, than a bug report, but this has been a major PITA for upgrading Google internal to TS 3.5, so I wanted to have a discussion here too.

Current Behavior
With TS3.4 pipe(filter(Boolean)) loses type information, everything after it is of type any. See stackblitz below:

const source = of('World').pipe(
  map(x => `Hello ${x}!`),
  filter(Boolean),
  map(y => y.madeup)  // no type error
);

The type of Boolean in lib.d.ts is

interface BooleanConstructor {
    new (value?: any): Boolean;
    (value?: any): boolean;
    readonly prototype: Boolean;
}

With TS3.5 Boolean is typed as

interface BooleanConstructor {
    new (value?: any): Boolean;
    <T>(value?: T): boolean;
    readonly prototype: Boolean;
}

https://github.com/microsoft/TypeScript/blob/master/lib/lib.es5.d.ts#L536

However, in most cases it appears <T> is not inferrable (I don't blame TS, this is really round-about), so TS3.5 pick the new default unknown, which in turn makes the type for the next operation unknown from previously any causing many compilation failures.

Reproduction
https://stackblitz.com/edit/rxjs-erah82

Possible Solution
I have been recommending replacing filter(Boolean) with the following more explicit workaround filter((x): x is OutputType => !!x) where OutputType is explicitly spelled out the expected type in the next operation in the chain.

Or maybe once microsoft/TypeScript#16655 (comment) lands filter(Boolean) will just work for Rxjs.

@cartant

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

Thanks. This is worthy of a linting rule that includes a fixer, IMO.

@cartant

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2019

Seems to be solved by adding the following overload signature (before the others):

export function filter<T>(predicate: BooleanConstructor): OperatorFunction<T, NonNullable<T>>;

ATM, I cannot see any downsides to this. The problem also affects find, first and last, AFAICT, so they would need an additional, initial overload, too.

IIRC, NonNullable was introduced in TS 2.8, so the additional signature should be okay in v6.

cartant added a commit to cartant/rxjs that referenced this issue Aug 11, 2019
@rkirov

This comment has been minimized.

Copy link
Author

commented Aug 12, 2019

Hmm, I did a small test with this change over our codebase and while some issues go away, there is another problem with type inference. In the simplest form this looks like:

interface I {
  a: string | null;
}

let i$: Observable<I> = of();
let s$: Observable<string> = i$.pipe(
  map(i => i.a),  // type-error here
  filter(Boolean) 
);

see
https://codesandbox.io/s/typescript-playground-lchy6?fontsize=14

It appears that TS's inferencer works "backwards" from the declared type - Observable<string>
thus picking T = string instead of T = string | null for the type parameter of filter and the return type of map, causing a type error. This might make this change hard to land and add some new confusion :(

@rkirov

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@alxhub came up with a fix for this situtation, by explicitly adding the |null|undefined types after the inferencer has picked some T.

function filterB<T>(
  predicate: BooleanConstructor
): OperatorFunction<T|null|undefined, NonNullable<T>> {
  return filter(predicate);
}

I will do another test to see if this fixes most breakages.

cartant added a commit to cartant/rxjs that referenced this issue Aug 15, 2019
benlesh added a commit that referenced this issue Aug 26, 2019
fix(types): add Boolean signature to filter (#4961)
* fix(types): add Boolean signature to filter

Closes #4959

* chore: add test using comment snippet

#4959 (comment)
#4968
@benlesh

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@rkirov i'm afraid that what you've outlined above doesn't really have a fix that I can see. Were you able to work around it on your end?

@rkirov

This comment has been minimized.

Copy link
Author

commented Aug 27, 2019

I tested the change outlined above in g3 and it passes, so we should be good. Also we upgraded to TS3.5, by removing all offending filter(Boolean)s, so it's all good.

My only hesitation is that with the change, one will always be allowed to do a filter boolean on a non-nullable type:

of(1,2,3).pipe(filter(Boolean)); // it would nice if that errs.

There is an argument for wanting that to be statically disallowed, but one can't have it all. Not having hidden 'any's any longer is a major win.

@cartant

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

of(1,2,3).pipe(filter(Boolean)) is valid, though, as zeroes will be filtered. Similarly, with strings, empty strings will be filtered. The joy of falsy values.

@rkirov

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

Right, I should have said "of({a: ''}, {a: 'foo'}).pipe(filter(Boolean)) should be an error", but yeah, that's too much to ask of the type system. I think this is a good change, thanks for fixing it quickly.

@cartant

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2019

What is the consensus on what @rkirov suggested above regarding the inclusion of T|null|undefined in the returned type?

function filterB<T>(
  predicate: BooleanConstructor
): OperatorFunction<T|null|undefined, NonNullable<T>> {
//                  ~~~~~~~~~~~~~~~~
  return filter(predicate);
}

Is this something we should be including in this change? @benlesh ?

IMO, this is okay, if it fixes a problem.

@rkirov

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

I didn't realize it didn't land with that modification. Without it, it will break internal users and with it there are no errors.

@cartant

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2019

tobi-or-not-tobi added a commit to SAP/cloud-commerce-spartacus-storefront that referenced this issue Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.