feat(groupBy): Support named arguments, support ObservableInputs for duration selector #5679

benlesh · 2020-08-25T23:04:11Z

Adds support for named arguments.
Adds support for returning promises, et al, from the duration selector.

NOTES:

There's a problem with type inferrence from the options duration selector that is noted in a TODO in the code here.
The tests for groupBy appear to be EXTREMELY old and outdated, and I was unable to updated them easily to use run mode. We may have to rewrite them all at some point to use better techniques. The issue seems to be a rudementary means of testing the inner observables that is incompatible with run mode.
Docs still need updated
Older paths still need to be deprecated
dtslint tests need to be added

benlesh · 2020-08-25T23:05:06Z

@kolodny or @cartant any ideas on the typings issue I found here? It's sort of strange. Basically, I can't get it to properly infer the type for the duration selector in GroupByOptions.

cartant · 2020-08-26T13:21:48Z

@benlesh IDK what is going on with the inference. If I hover over the duration key the inference seems to have been made correctly:

But if I hover over the group parameter, it's not - K is inferred to be unknown:

FWIW, this is in VS Code using TS 4.0.2.

voliva · 2020-08-26T15:39:22Z

Hey, I think this is an issue in Typescript.

Note that if, as a consumer, you set the type of the parameter for the key selector:

Then it picks up the type correctly. The error shown is then because key is a string, but durations is an array. Also note that the only relevant bit is specifying the type of the parameter in the key selector - the return value doesn't affect this case.

I remembered this because I had the same problem some years ago, in a very similar interface, and raised this issue microsoft/TypeScript#25092.
That one is closed now, and they're tracking the meta-issue (as it causes many different ones) here: microsoft/TypeScript#30134

cartant · 2020-08-26T21:52:04Z

@benlesh I don't think this is related to the issues @voliva referenced - there is no free type parameter here, T is inferred from the source observable upon which pipe is called; it's not free.

The fact that the inference is made if (val: string) is specified, suggests to me that - when val is not explicitly typed - TS is abandoning inferring K from the key arrow function when it sees that the type of val needs to be inferred. However, it's T and T is known - from the pipe call site.

Additionally, if a temporary property k: () => K is added to GroupByOptions and k: () => "" is added to the test, inference works as you would expect.

Consequently, I agree with Victor that this appears to be a TS issue.

The fact that the popup shows that the inference of K is made in one place and not in another suggests to me that the TS language service and compiler are behaving differently.

cartant · 2020-08-26T22:22:53Z

There is a TS playground repro here. Note that the discrepancy between popups isn't present in the playground.

voliva · 2020-08-26T22:57:33Z

I'm sorry to insist (and for the extra noise when it probably doesn't really matter) - But I still think that it's essentially the same issue I refereneced.

Note that in that issue, there's also no free type parameter:

interface MyInterface<T> {
    retrieveGeneric: (parameter: string) => T,
    operateWithGeneric: (generic: T) => string
}

inferTypeFn({
    retrieveGeneric: parameter => 5,
    operateWithGeneric: generic => generic.toFixed() // Fails .toFixed() not in generic
});

Semantically, T should be infered to number.

Likewise, in GroupByOptions:

interface GroupByOptions<T, K> {
  key: (value: T) => K;
  duration?: (grouped: GroupedObservable<K, T>) => ObservableInput<any>;
  subject?: () => Subject<T>;
}
groupBy({
  key: val => val,
  duration: (group) => durations[group.key] // Fails `key` unknown
})

Semantically, K should be infered to 'string', but TS fails to do so.
The key selector in GroupByOptions is "retrieving the generic" K, and this K is later used in the duration selector, which would be the operateWithGeneric in the original example.

I reckon the only difference between these two examples is that the generic name that TS fails to properly infer is K in rxjs' case and T in the original issue's case. 2 years ago we didn't have unknown yet, that's why it infered {} instead. If you try the original issue in the current TS version, you'll also get unknown in operateWithGeneric

As an alternative, I'd suggest two options, but both have drawbacks :(

Ignore this error and let consumers handle it (e.g. casting to any or doing the redundant parameter definition workaround). It will get eventually™ fixed
have the key selector and the options in diferent parameters within groupBy (e.g. groupBy(keySelector, { duration, subject })

cartant · 2020-08-26T23:29:19Z

@voliva Yeah, I see what you mean regarding your issue. It's not immediately apparent why your issue was closed in favour of the issue that explicitly mentions free type parameters. The reason is given in this comment:

Based on what we've talked about in related issues before, it's unlikely we'll specifically improve this without moving to a full unification-based solve for the arguments.

The same comment also explains what's happening here:

We see [the parameter] unannotated first, lock in [unknown] as the inference

That seems to suggest it infers unknown for T from which infers the same for K, but then figures out T from the call site/context and uses that for T but doesn't 'go back' to infer K once T is known.

I guess this is not going to be fixed anytime soon. It's curious that the VS Code extension's tooling gets it right, though.

…duration selector - Adds support for named arguments. - Adds support for returning promises, et al, from the duration selector. NOTES: * There's a problem with type inferrence from the options `duration` selector that is noted in a `TODO` in the code here. * The tests for `groupBy` appear to be EXTREMELY old and outdated, and I was unable to updated them easily to use run mode. We may have to rewrite them all at some point to use better techniques. The issue seems to be a rudementary means of testing the inner observables that is incompatible with run mode. * Docs still need updated * Older paths still need to be deprecated * dtslint tests need to be added

benlesh · 2021-04-29T03:11:33Z

Well. I've updated this PR... however we still have the same issue. So. I'm not sure what we can do about it. Super weird.

voliva · 2021-04-29T05:46:20Z

I think both of these alternatives are still valid:

As an alternative, I'd suggest two options, but both have drawbacks :(

Ignore this error and let consumers handle it (e.g. casting to any or doing the redundant parameter definition workaround). It will get eventually™ fixed

have the key selector and the options in diferent parameters within groupBy (e.g. groupBy(keySelector, { duration, subject })

1 means that consumers will need to add the specific generics by themselves while the issue in TS is not fixed, e.g.

    obs.pipe(groupBy<string, string>({
      key: (val) => val,
      duration: (group) => durations[group.key]
    })).subscribe();

2 means changing the new options overload so that the duration selector is placed in a different parameter than the key selector, e.g.

    obs.pipe(groupBy(
      (val) => val,
      {
        duration: (group) => durations[group.key]
      }
    )).subscribe();

(note that in the test it will still complain but for a good reason: group.key is a string, but durations is an array which expects number indices)

On my personal preference, I'd go with (2). I think it also makes sense to always have the parameter be the key selector, and optionally take in an options object, while deprecating all the old overloads.

cartant · 2021-04-29T06:06:40Z

Definite 'no' for 1 ☝️ from me. 2 is fine, though.

Explicit type parameters - i.e. type parameters as type assertions - are something to be avoided wherever possible, IMO, and if they - or an explicit type annotation - are necessary to work around a shortcoming in TypeScript's behaviour, I would like to avoid it. It's just going to confuse people and they are going to open an issue, etc.

Anyway, the key selector always needs to be specified, so if it's a separate argument, it's very much like the connect API - a selector parameter followed by a completely optional config parameter - so my vote would be for number two.

benlesh · 2021-04-29T13:41:39Z

Okay, I've switched to groupBy(fn, options) which I sort of like better, because I was planning on leaving groupBy(fn) without deprecations, since it's the most common pattern.

cartant · 2021-04-29T22:54:24Z

spec/operators/groupBy-spec.ts

    const source = e1
-      .pipe(groupBy(
-        (val: string) => val.toLowerCase().trim(),
-        (val: string) => val,
-        (group: any) => group.pipe(skip(2))
-      ));
+      .pipe(
+        groupBy(val => val.toLowerCase().trim(), {
+          duration: group => group.pipe(skip(2)),
+        })
+      );


I can see that the element selector isn't really necessary based on the test's description, but with the removal of the element selectors there are now no tests that specify them. Maybe we should add one?

cartant · 2021-04-29T22:56:19Z

src/internal/operators/groupBy.ts

+export function groupBy<T, K, E>(
+  key: (value: T) => K,
+  options: GroupByOptionsWithElement<K, E, T>
+): OperatorFunction<T, GroupedObservable<K, E>>;
+
+export function groupBy<T, K, E>(
+  key: (value: T) => K,
+  options: GroupByOptionsWithElement<K, E, T>
+): OperatorFunction<T, GroupedObservable<K, E>>;


AFAICT, these signatures are the same?

Oops, I must have refactored one and not noticed.

cartant · 2021-04-29T23:00:01Z

src/internal/operators/groupBy.ts

+    if (!elementOrOptions || typeof elementOrOptions === 'function') {
+      element = elementOrOptions;
+    } else {
+      ({ duration, element, connector } = elementOrOptions);


Destructuring assignment FTW. 🎉❤

benlesh requested review from kolodny and cartant August 25, 2020 23:05

cartant mentioned this pull request Mar 14, 2021

DistinctUntilChanged keyselector typing issue #5484

Open

benlesh marked this pull request as ready for review April 29, 2021 01:49

benlesh force-pushed the feat/groupBy-named-args branch from b21d811 to 9c26c40 Compare April 29, 2021 03:01

chore: rebased. There are still type issues

9c26c40

refactor: change to fn, options pattern

2ac05da

benlesh changed the title ~~WIP: feat(groupBy): Support named arguments, support ObservableInputs for duration selector~~ feat(groupBy): Support named arguments, support ObservableInputs for duration selector Apr 29, 2021

cartant approved these changes Apr 29, 2021

View reviewed changes

cartant added the 7.x Issues and PRs for version 7.x label May 5, 2021

benlesh mentioned this pull request May 5, 2021

Version 7 -> 8 Roadmap #6367

Open

40 tasks

chore: remove duplicate typing

c376360

benlesh merged commit 7a99397 into ReactiveX:master May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(groupBy): Support named arguments, support ObservableInputs for duration selector #5679

feat(groupBy): Support named arguments, support ObservableInputs for duration selector #5679

benlesh commented Aug 25, 2020

benlesh commented Aug 25, 2020

cartant commented Aug 26, 2020

voliva commented Aug 26, 2020 •

edited

Loading

cartant commented Aug 26, 2020 •

edited

Loading

cartant commented Aug 26, 2020

voliva commented Aug 26, 2020 •

edited

Loading

cartant commented Aug 26, 2020 •

edited

Loading

benlesh commented Apr 29, 2021

voliva commented Apr 29, 2021 •

edited

Loading

cartant commented Apr 29, 2021 •

edited

Loading

benlesh commented Apr 29, 2021

cartant Apr 29, 2021

cartant Apr 29, 2021

benlesh May 5, 2021

cartant Apr 29, 2021 •

edited

Loading

feat(groupBy): Support named arguments, support ObservableInputs for duration selector #5679

feat(groupBy): Support named arguments, support ObservableInputs for duration selector #5679

Conversation

benlesh commented Aug 25, 2020

benlesh commented Aug 25, 2020

cartant commented Aug 26, 2020

voliva commented Aug 26, 2020 • edited Loading

cartant commented Aug 26, 2020 • edited Loading

cartant commented Aug 26, 2020

voliva commented Aug 26, 2020 • edited Loading

cartant commented Aug 26, 2020 • edited Loading

benlesh commented Apr 29, 2021

voliva commented Apr 29, 2021 • edited Loading

cartant commented Apr 29, 2021 • edited Loading

benlesh commented Apr 29, 2021

cartant Apr 29, 2021

Choose a reason for hiding this comment

cartant Apr 29, 2021

Choose a reason for hiding this comment

benlesh May 5, 2021

Choose a reason for hiding this comment

cartant Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

voliva commented Aug 26, 2020 •

edited

Loading

cartant commented Aug 26, 2020 •

edited

Loading

voliva commented Aug 26, 2020 •

edited

Loading

cartant commented Aug 26, 2020 •

edited

Loading

voliva commented Apr 29, 2021 •

edited

Loading

cartant commented Apr 29, 2021 •

edited

Loading

cartant Apr 29, 2021 •

edited

Loading