Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Switch to config objects for operators with a lot of arguments. #3515

Closed
benlesh opened this issue Apr 2, 2018 · 4 comments
Closed

Switch to config objects for operators with a lot of arguments. #3515

benlesh opened this issue Apr 2, 2018 · 4 comments

Comments

@benlesh
Copy link
Member

benlesh commented Apr 2, 2018

There are a few operators that have a lot of arguments, one example would be groupBy. This hurts readability a bit and also bites us later if we want to change or deprecate arguments.

So the idea is simple, introduce non-breaking changes to operators like groupBy to allow the passing of a config object to essentially provide "named arguments":

groupBy(fn1, fn2 fn3, fn4)

// becomes

groupBy({
  keySelector: fn1,
  resultSelector: fn2,
  durationSelector: fn3,
  subjectFactory: fn4,
})

(The names can be bikeshedded later, ideally to make it more terse but still readable)

@benlesh benlesh added type: discussion AGENDA ITEM Flagged for discussion at core team meetings labels Apr 2, 2018
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label May 23, 2018
@BioPhoton
Copy link
Contributor

BioPhoton commented Oct 3, 2019

Hi @benlesh!

If this is still a thing I would create a list of all the operators that have enough arguments to get refactored to a config object.

I would collect:

  • their name
  • link to the source file
  • their argument names and roughly the types
  • suggestion for future naming (just as a placeholder. The real name is up to people that are responsible for it)
  • structure of the future config object

I would provide it as markdown like this:

  • groupBy
    Current Arguments:

    • keySelector: Function
    • elementSelector: Function
    • durationSelector: Function
    • subjectSelector: Function

    Future Config Object

    • Name: groupingConfig
    • Structure:
    {
      keySelector: Function
      elementSelector: Function
      durationSelector: Function
      subjectSelector: Function
    }
  • otherOperator
    Current Arguments:

    • ...

    Future Config Object

    • ...

Let me know if the suggested approach would be helpful or if I can adapt anything.

@benlesh
Copy link
Member Author

benlesh commented Oct 3, 2019

Thanks @BioPhoton.... we probably don't even need that level of detail. Just a list of the operators that have 3 or more arguments would be a great start.

@BioPhoton
Copy link
Contributor

Could you suggest a naming pattern?
I.e. having Config as postfix or similar?

@BioPhoton
Copy link
Contributor

Hi @benlesh!

Here the list of operators/functions that have 3 or more arguments (n means infinite)

Subjects

  • ReplaySubject (3)

Creation Methode

  • bindCallback (3)
  • bindNodeCallback (3)
  • combineLatest (n)
  • concat (n)
  • forkJoin (n)
  • fromEvent (4)
  • fromEventPattern (4)
  • iif (3)
  • merge (n)
  • of (n)
  • onErrorResumeNext (n)
  • race (n)
  • range (3)
  • timer (3)
  • zip (n)

Operators

  • combineLatest (n)
  • concat (n)
  • count (3)
  • endWith (n)
  • every (3)
  • expand (3)
  • find (3)
  • findIndex (3)
  • groupBy (4)
  • merge (n)
  • mergeMap (3)
  • mergeMapTo (3)
  • mergeScan (3)
  • onErrorResumeNext (n)
  • pluck (n)
  • publishReplay (3)
  • race (n)
  • shareReplay (3)
  • single (3)
  • startWith (n)
  • tap (3)
  • throttleTime (3)
  • timeoutWith (3)
  • windowTime (4)
  • withLatestFrom (n)
  • zip (n)

@benlesh benlesh closed this as completed May 4, 2021
@ReactiveX ReactiveX locked and limited conversation to collaborators May 4, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants