Allow tap operator after takeUntil

[willium]

Give that tap's contract is that it can perform side effects but returns an identical Observable, is it not safe to follow a takeUntil with a tap (and thus allow it here)

e.g.

const stream$ = of().pipe(
    ...
    takeUntil(unsubscribe$),
    tap((datasets) => { setSomeState(true) })
)

[cartant]

That's not the point of the rule, though. The idea is that takeUntil ought to be placed as the last operator in the pipeline unless there is a compelling reason to not do so. There's no reason the tap cannot be placed before the takeUntil.

[willium]

Well in this case, if the tap were before it could cause a bad state update on an unmounted component, no?

[cartant]

No. When the takeUntil notifier emits, the pipeline will unsubscribe.

[willium]

Ah—even if its in the middle of executing the pipeline (and the tap) operator?

[cartant]

Well, there's only one thread - for which I'm kinda grateful, TBH.

[cartant]

If the tap was using a complete callback, it would behave differently, though. In that case, you might want to disable the rule locally using an /* eslint-disable-next-line ... */ comment. A lot these rules try to help devs by performing some static analysis to find things that might be problematic, but if you find a situation that warrants it, disable the rule locally.

[willium]

Ha—appreciate your help! It sounds like I'm just missing something—or thinking too imperatively—but by my read it seems like the side effects in the tap would occur before the pipe "gets" to the takeUntil and thus "checks" to see that the notifier hadn't emitted... but now writing that here it seems like I've got pipes backwards... that the notifier would emit when the .next() and .complete() calls are made. As you said, there is only 1 thread, and it's not possible that there was some kind of race condition. Is that right?

[cartant]

... and it's not possible that there was some kind of race condition. Is that right?

Yeah, there's no race here, the unsubscription is synchronous.

[willium]

A lot these rules try to help devs by performing some static analysis

Totally! I'm a relative newcomer to RxJS, so I don't want to go turning off linting rules that make sense unless I understand them. Despite the fact that you've now convinced me it's not possible for the side-effects in a tap proceeding a takeUntil to cause problems, it still feels like the eslint rule could allow for tap operators to follow a takeUntil as it seems to not have any adverse effects or leaky cases? It feels a bit wrong for the linting rule to be more strict than the contract we have with the tap function not to effect the Observable at all, no?

[cartant]

It's essentially the way it is 'cause it's easier to tell people that takeUntil should generally be placed last. That's the general rule of thumb, but if you needed a toArray - or something else that depends on a complete notification - then that operator would need to go last (the rule accounts for this).

I think it's easier to explain that takeUntil is best placed last than it is to explain which operators can leak inner subscriptions if placed after the takeUntil. So it's not that the tap is unsafe, it's just not easy to explain/remember which operators are unsafe.

[willium]

Thanks for the explanation! appreciate your patience :)

[cartant]

No worries. At some stage, I'll see if I can make some changes to the docs - and/or failure messages - to better explain what's going on.