rxjs icon indicating copy to clipboard operation
rxjs copied to clipboard

feat(groupBy): add seed option to eagerly create groups

Open backbone87 opened this issue 3 years ago • 5 comments

  • [ ] It must have a -spec.ts tests file covering the canonical corner cases, with marble diagram tests
  • [ ] The spec file should have a type definition test at the end of the spec to verify type definition for various use cases
  • [ ] The operator should also be documented. See Documentation Guidelines.

Description:

adds a seed option to the groupBy operator which allows opening groups eagerly.

if the approach is agreed upon I will provide more tests.

Related issue (if exists): https://github.com/ReactiveX/rxjs/pull/6388#issuecomment-846192168

backbone87 avatar May 25 '21 22:05 backbone87

I would not be in favour of this, especially as seed is () => ObservableInput<K>. That doesn't fit with this description of the feature:

opening groups eagerly

I'm also unconvinced there is a compelling use case for this.

cartant avatar May 25 '21 22:05 cartant

the use case is what buckets/splitBy would have enabled:

number$.pipe(
  groupBy(isEven, { seed: booleans }),
  toArray(),
  mergeMap(([evenNumber$, oddNumber$]) =>
    merge(
      evenNumber$.pipe(mapTo('I am even')),
      oddNumber$.pipe(mapTo('I am odd')),
    )
  ),
);

edit: I used an observable factory for flexibility, so it can be controlled when to open groups and until when groups are automatically opened.

backbone87 avatar May 25 '21 22:05 backbone87

I guess the important bit that @cartant is bringing up is the use cases this fulfills. One of the issues we already have with this library is we have a lot of APIs that simply don't have a ton of use cases. groupBy borders on that already depending on who you ask.

The thing to keep in mind is: Every new feature we introduce we have to MAINTAIN FOR YEARS. So we need to be sure there's value to it.

benlesh avatar May 27 '21 20:05 benlesh

Every new feature we introduce we have to MAINTAIN FOR YEARS. So we need to be sure there's value to it.

Absolutely no problem with this. I just want the proposal to be given fair consideration and a bit of discussion (edit: And of course this may take its time depending on the availability of the maintainers and others who may want to voice their opinions). If the conclusion is to not include the PR, so it be.

groupBy borders on that already depending on who you ask.

As probably one of the few who use rxjs not in the frontend, but in the backend for event stream processing, I can tell that groupBy is one of the main workhorses for me. Sure that is anecdotal, but still.

backbone87 avatar May 27 '21 23:05 backbone87

any news on this?? I need to use it

spoilerdo avatar Oct 12 '22 13:10 spoilerdo