CODEWITHSUNDEEP
javascriptrxjs
azerafati
azerafati

Reputation: 18583

Is subscribing inside RxJS tap, bad practice for an asynchronous logic?

I am having two http requests where the second one (Log request) should be subscribed after the first one (Order request) emits a value and I do some logic that should not be blocked by the Log request and the result of log request is ignored.

As I understand that tap is

Used to perform side-effects

https://rxjs.dev/api/operators/tap

and since I assume Log request and its response here is a side-effect to the Order request, is it bad practice to subscribe inside tap? Is there a more slick and RxJS way of handling this?

const order = of('2- created order').pipe(
  delay(100),
  tap(console.log)
);

const log = of('4- logged info to log server').pipe(
  delay(500),
  tap(console.log)
);

console.log('1- started creating order');
order
  .pipe(tap(() => log.subscribe()))
  .subscribe(() => console.log("3- didn't wait for log server"));

StackBlitz

Upvotes: 2

Views: 3615

Answers (2)

BizzyBob
BizzyBob

Reputation: 14740

As @martin mentions in the comment, it's generally bad to subscribe inside a pipe (not specifically tap, but any operator) because there's no way handle cleaning up subscriptions.

Generally, it's preferred to use one of the "Higher-Order Mapping Operators" because they handle subscribing, emitting, and unsubscribing from your observable.

Is there a more slick and RxJS way of handling this

Not sure if this would be considered "slick" or not :-), but I think it gives a nice separation of concerns if a Subject is used as a dedicated stream of log messages; then create a single subscription to have your server logging logic executed:

const logMessage$ = new Subject<string>();
logMessage$
  .pipe(mergeMap(logToServer))
  .subscribe();

Then in your other code, instead of subscribing, you can call logMessage$.next() to trigger the server logging logic without impeding the flow of your order stream:

order.pipe(tap(o => logMessage$.next(o)));

Here's an updated StackBlitz.

Upvotes: 2

Dario Piotrowicz
Dario Piotrowicz

Reputation: 1102

Yes, it definitely is bad practice.

You're right that tap is there for side effects but those should not involve other streams, they should be simple side effects like assigning variables or console logging and stuff.

The issue is that you generally do not want to subscribe inside a subscribe on inside the pipe because doing so makes for very unpredictable and difficult to maintain code.

For example your subscribe inside the tab looks innocuous enough, but imagine if you were listening to a continuous stream of data, how many subscriptions would you have? would you care to track them all to unsubscribe, wouldn't that end up quite difficult to understand/debug, etc...

The problem with your code is that you are somewhat thinking in an imperative way (something like "do this and then that, and then...") instead of thinking in terms of streams.

So basically in my opinion, instead of thinking something like "how do I do this before that?" you should be thinking about how you handle the stream and the order of operations you can perform on it.

In your case, is there any reason why you'd want to print the third message inside the subscribe and not in the pipe?

Why not just doing the following?

order
  .pipe(
    tap(() => console.log("3- didn't wait for log server")),
    switchMap(() => log)
  )
  .subscribe();

(like here: https://stackblitz.com/edit/rxjs-aace8i?file=index.ts)

If I may, I would like to analyse your question...

Starting with

I am having two http requests where the second one (Log request) should be subscribed after the first one (Order request) emits a value

that just seems a simple case of having an initial observable (Order) and needing to use a mapping operator to move to a different one (Log), I assumed that you want to discard the first and move to the second so I chose switchMap (alternatively you could use concatMap or mergeMap)

Then we get to:

and I do some logic that should not be blocked by the Log request and the result of log request is ignored.

since we already thought of how to handle the 2 observables, if we read your sentence it really spells out that we just want a side effect to occur between the first and second observable, and it ignores the streams' values anyways, so it clearly calls for a simple tap

I am sorry for the rather long message I hope it does not sound too pedantic :P

What I basically wanted to say is that you should always look at your streams and sort of think how to fit all together in accordance to rxjs programming style and what you need, and not whether some ways of doing something is acceptable or not, as that makes me realy thing that you already had suspicions on whether it not being the best solution.

Upvotes: 2

Related Questions