Should Subscribable fire hooks only on actual insert or remove ?

[LeDevNovice]

Package: @tanstack/query-core
Area: Subscribable base class

Hi guys !

I’ve been exploring the Subscribable base class today and I’m wondering about the intended semantics of its hooks and the unsubscribe function. I’d love maintainer feedback on the following questions.

1. Should onSubscribe() fire only when a new listener is actually inserted ?

Actually, I understand that subscribe() calls onSubscribe() unconditionally after Set.add(listener). If the same function reference is subscribed twice, Set.add won’t grow the set, yet onSubscribe() still fires.

Question: Is it expected (by design) that onSubscribe() can fire even when the listener set didn’t change ?

2. Should onUnsubscribe() fire only when a listener is actually removed ?

The returned unsubscribe() currently invokes onUnsubscribe() unconditionally after Set.delete(listener).

If unsubscribe() is called twice, the second Set.delete returns false, yet onUnsubscribe() still fires.

Question: Is it intended that onUnsubscribe() may fire when nothing was removed ?

Should the hooks reflect actual insert or remove operations, or is the current behavior intentionally looser ?

Would a minimal adjustment like this make sense?

export class Subscribable<TListener extends Function> {
  protected listeners = new Set<TListener>()

  constructor() {
    this.subscribe = this.subscribe.bind(this)
  }

  subscribe(listener: TListener): () => void {
    const sizeBefore = this.listeners.size
    this.listeners.add(listener)
    if (this.listeners.size > sizeBefore) {
      this.onSubscribe() // only when we actually inserted
    }

[TkDodo]

I think you’re probably technically correct that we shouldn’t call those event listeners if nothing gets actually added or removed, but we also don’t need it for our use-cases so I don’t think we should add more code to guard against things that aren’t happening in our code-base (like calling subscribe multiple times). This isn’t a generic implementation but one for our specific needs.

[LeDevNovice]

That makes sense, thanks for the clarification Dominik.