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

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

backbone87
Copy link
Contributor

  • 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):
#6388 (comment)

@cartant
Copy link
Collaborator

cartant commented May 25, 2021

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.

@backbone87
Copy link
Contributor Author

backbone87 commented May 25, 2021

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.

@benlesh
Copy link
Member

benlesh commented May 27, 2021

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.

@backbone87
Copy link
Contributor Author

backbone87 commented May 27, 2021

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 backbone87 mentioned this pull request Jun 21, 2021
@spoilerdo
Copy link

any news on this?? I need to use it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants