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

groupBy should support typeguards #5439

Closed
benlesh opened this issue May 15, 2020 · 2 comments
Closed

groupBy should support typeguards #5439

benlesh opened this issue May 15, 2020 · 2 comments
Assignees

Comments

@benlesh
Copy link
Member

benlesh commented May 15, 2020

I'm not even sure this is possible, but it would be great if this "just worked":

function isNumber(input: any): input is number {
   return typeof input === 'number';
}

of('a', 1, 'b', 2).pipe(
   groupBy(isNumber),
   mergeMap(group => {
     if (group.key) {
         return group; // Observable<number>
     } else {
         return group; // Observable<string>
     }
   });
)
@benlesh benlesh assigned cartant and kolodny and unassigned cartant May 15, 2020
@cartant
Copy link
Collaborator

cartant commented May 15, 2020

This sounds like splitBy - see #3807 - which I implemented here but I didn't open a PR for it - I guess I wasn't entirely happy with it. I can take another look.

@cartant
Copy link
Collaborator

cartant commented May 15, 2020

Hmm, this is a little weird. It would seem that adding this signature should do it:

  export function groupBy<T, K extends T>(
    keySelector: (value: T) => value is K
  ): OperatorFunction<T, GroupedObservable<true, K> | GroupedObservable<false, T>>;

And the key property needs to be made const:

constructor(public key: K,

However, the type is only narrowed within the if and not the else - which is weird:

of("a", 1, "b", 2).pipe(
  groupBy(isNumber),
  mergeMap(group => {
    if (group.key) {
      return group; // GroupedObservable<true, number>
    } else {
      return group; // GroupedObservable<true, number> | GroupedObservable<false, any>
    }
  })
);

It looks like the inference isn't taking into account that the type of key is true | false which is a boolean.

@benlesh benlesh closed this as completed May 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants