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

Default values for do() parameters #2534

Closed
felixfbecker opened this issue Apr 9, 2017 · 11 comments · Fixed by #3073, GulajavaMinistudio/rxjs#75 or Xenira/Shashki#66
Closed

Default values for do() parameters #2534

felixfbecker opened this issue Apr 9, 2017 · 11 comments · Fixed by #3073, GulajavaMinistudio/rxjs#75 or Xenira/Shashki#66
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@felixfbecker
Copy link
Contributor

felixfbecker commented Apr 9, 2017

promise.then() and subscribe() can be called with passing undefined as the first parameter, e.g. to only handle errors or completions. do() requires to pass a noop function like () => undefined. It should just have this as default value.

doOnComplete and doOnError shorthands would also be nice.

@midnight-wonderer
Copy link
Contributor

@felixfbecker
Copy link
Contributor Author

Not according to the signature:

export declare function _do<T>(this: Observable<T>, next: (x: T) => void, error?: (e: any) => void, complete?: () => void): Observable<T>;

@david-driscoll
Copy link
Member

It's entirely possible that the signature is incorrect, however it feels like a corner case. What would someone be trying to accomplish by calling do() by itself?

As to doOnComplete and doOnError you can easily do do({ error(e) { /**/ } }); or `do({ complete() {} });

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Apr 9, 2017

For example to instrument a function with OpenTracing:

const span = childOf.tracer().startSpan('my operation', { childOf })
return someAction()
  // ... apply operators ...
  .do(undefined, err => {
    this.logger.error(err);
    span.setTag('error', true);
    span.log({ 'event': 'error', 'error.object': err });
  })
  .finally(() => {
    span.finish();
  })

this is personal taste of course, but I find the object version less readable:

const span = childOf.tracer().startSpan('my operation', { childOf })
return someAction()
  // ... apply operators ...
  .do({ error: err => {
    this.logger.error(err);
    span.setTag('error', true);
    span.log({ 'event': 'error', 'error.object': err });
  }})
  .finally(() => {
    span.finish();
  })

@david-driscoll
Copy link
Member

Unfortunately I can safely say that the shorthand operators for do (and their counter parts for subscribe) probably won't be making it back into the core library. We already have enough operator overload with the current number, adding more will just make things more confusing for others.

However you could easily make your own using augmentation for your application / library. You could even make it an npm package if you wanted! 🎉

See:
https://github.com/ReactiveX/rxjs/blob/bc1e1e53897aeca61288378cc8e2b33ed20dbbcf/src/add/operator/do.ts

@kwonoj
Copy link
Member

kwonoj commented Apr 9, 2017

as same as @david-driscoll , I don't think we'll bring back variant of doOn.. but making signature of do to make next to optional seems considerable option. Only thing I'd like to have is if there's some way to make partial param must have one at least (either next, error, or complete) which seems not trivial.

@felixfbecker
Copy link
Contributor Author

you could add overloads for all 2^3 cases. But why would you want this? Can't you just return the source if no argument is passed? Where's the benefit to restricting it?

@kwonoj
Copy link
Member

kwonoj commented Apr 9, 2017

Functionally it'll work as you'd described. Reason for this is mostly about explicitness of api definition signature - compare to subscribe doesn't have any observer contract param, do is all about triggering side effect by provided handler and would not like to implicit assumption you could do do() which actualy doesn't do anything. We're trying to make TS definition as correct api signature even JS user can refer it to clarify behavior of api.

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label Apr 9, 2017
@artaommahe
Copy link

artaommahe commented May 21, 2017

this do(null, () => null) does not work with strictNullChecks enabled

[ts] Argument of type 'null' is not assignable to parameter of type '(x: {}) => void'.

felixfbecker added a commit to felixfbecker/rxjs that referenced this issue Nov 14, 2017
felixfbecker added a commit to felixfbecker/rxjs that referenced this issue Nov 14, 2017
benlesh pushed a commit that referenced this issue Nov 15, 2017
@martinsik
Copy link
Contributor

@artaommahe You can use undefined instead:

do(undefined, () => null)

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
6 participants