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

[shairez]

CONTEXT:

Following #12 (by the amazing @ThomasBurleson) we had a discussion and realized this PR could be separated into several PRs, and before that we need to discuss them.

This one is about: the possible changes to the ObserverSpy API to make it more "getters" focused.

The suggested API changes to ObserverSpy -

export type CompletionCallback = (spy: ObserverSpy<any>) => void;
const NOOP = () => {};

export class ObserverSpy<T> implements Observer<T> {

	constructor(private onCompleteCallback: CompletionCallback = NOOP) {}

...

	complete(): void {
    	this._state.called.complete = true;
	    this.onCompleteCallback(this);
	  }

...

The upsides (IMO) -

• You define it one time and cannot change it afterwards (might reduce confusion?)

The downsides (IMO) -

• It forces you to write your expectations before you trigger your action, meaning your test could end up looking like this:

it('should do something', ()=>{
	// SETUP
	const observerSpy = new ObserverSpy((spy)=>{
		// OUTCOME
		expect(spy.getValuesLength()).toBe(2);
	})

	// ACTION
	observableUnderTest.subscribe(observerSpy);

	// I personally don't like this style, I prefer to follow the "setup, action, outcome" structure in that order
	// because it keeps the tests more readable and predictable IMO.
});

• It introduces 2 ways to do 1 thing, which is something I'd like to avoid to keep the library as simple as possible so people won't have to guess "what's the best way".
  Plus, if you keep the onComplete method, you then have another discussion to have - "what happens when you define both callbacks? do they co-exist or not?"

---

I'd love to hear your thoughts / corrections @ThomasBurleson @katharinakoal @edbzn @burkybang @yjaaidi (and whoever is interested), before we decide whether to break it into its own PR.

SO... WHAT DO YOU THINK?

[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?

[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.

[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...

[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