DISCUSSION 2: ObserverSpy API - passing `OnComplete` callback in the constructor?

[hirezio]

DISCUSSION 2: ObserverSpy API - passing `OnComplete` callback in the constructor?

[shairez]

Thanks @katharinakoal @NetanelBasal @yjaaidi for your feedback!

Younes - yeah, I agree with returning a promise
That's what was solved by @edbzn 's PR #3

The original observerSpy.onComplete() callback was added because I just didn't think about async-await.
And that's why I love open-source so much 😄

One use case I can think of is maybe older code that doesn't support async-await or promises, but I don't think it's that common, so we might just remove it in the future...

[NetanelBasal]

I'm not sure what's the use case for this hook. In addition to that, if you add the onComplete hook, it makes sense to be consistent and expose onError and onNext hooks.

[yjaaidi]

Solution: I wouldn't implement this feature. 😁

Alternative: I would replace by it by .whenComplete(): Promise and only document usage for await observer.whenComplete().

Reason: I think that tests should never use callbacks because:

• they makes tests harder to read
• most users might forget to call done()
• allowing callbacks encourages waterfall
• it kind of breaks one of the purposes of ObserverSpy which is to avoid assertions in the subscribe

If we keep it, I would expect it to be an Observable so I can use it this way observer.onComplete.subscribe(completeObserverSpy) 😜

I might miss some edge case where a callback is easier to read... but I can't figure it out.

[katharinakoal]

I want to add a point to the downsides:
When overloading the constructor, it might be confusing to "guess" how the argument influences the instance.
This could be solved by adding a static factory method like this:
const spy = ObserverSpy.withCompleteCallback(fn)

But then again, it's getting a bit complex. Maybe it's worth it, and I'm just missing some other advantages?

[edbzn]

Hi, agree with @yjaaidi about using Promises, it's always better! Maybe we can deprecate the onComplete(fn) overload and remove the callback support later.

[shairez]

Thanks for the feedback @edbzn !

I thought about maybe another potential reason - whether using promises has any performance implications (microtasks queue wise)

So maybe callbacks, for sync Observables, could have slightly better performance? (not sure, just an unchecked thought) and that could be a reason to keep them...

Any thoughts/insights about that?

[yjaaidi]

Interesting point 🤔
I don't think that this would have dramatic performance impact. I would worry more about tests that want to check if some operator like observeOn is triggering a microtask or running synchronously... but it's an extreme edge case, in my opinion.

Otherwise, I'd focus more on simplicity and reducing the library's surface than the impact of an almost superfluous teeny-weeny micro tasky 😊

[shairez]

Finally I had some time to get back to these issues 😅

I'm closing this issue as it seems like the majority thinks it's better to stay with async await (promises).

Thanks everyone for participating!

BTW, You can view the latest release with @ThomasBurleson 's improvements here -

https://github.com/hirezio/observer-spy/releases/tag/v1.4.0