Refactor returning `clear` functions from reactive primitives

[thetarnav]

Currently our event-listener package and primitives that use it internally (e.g. mouse and active-element) are are following such pattern for removing the listeners. Even timer and date packages does that in some primitives.

const clear = createEventListener(el, "click");
clear()

This removes the listeners, but there is no easy way for restoring the listeners. It will happen once the reactive sources change, for example the target or eventName signals.

const [el, setEl] = createSignal(ref);
const clear = createEventListener(el, "click");
clear()
// will restore listeners:
setEl(ref2)

First this is clunky to do, especially if we don't want to change the element. And secondly this feels like an implicit behaviour, that can happen without us really planning it – the primitive stays reactive.

A possible solution:

Separate primitives into:

• lower level primitive that is responsible only for: 1. attaching the event listeners 2. removing them onCleanup 3. returning the clean function

  const clear = setEventListener(el, "click", (e) => {...})
  clear()

• higher level – reactive primitive, that wraps itself internally in an effect, allowing for accessor arguments, that will cause the listeners to be removed and attached on change. Doesn't return the clear function – has to wrapped in a createDisposable (#102) to be disposed early, because it is an effect.

  
  const [target, setTarget] = createSignal(el);
  createEventListener(target, "click", (e) => {...});

// or disposable: const clear = createDisposable(() => { createEventListener(target, "click", (e) => {...}); }) clear()

This refactor would need to be applied to all packages that share this pattern.

[otonashixav]

Another option: return createEventEffect already in a branch if it is reactive, which keeps clear on everything without the implicit restart behaviour:

const createEventEffect = (...) => {
  if (!reactive) return createEventListener(...);
  // if reactive, deopt and createBranch
  return createDisposable(() => { ... });
}

Which trades performance and implementation simplicity for a cleaner and simpler API. An advantage is it avoids createDisposable or createBranch; I mention both because with primitives that return an accessor:

let accessor!: SomethingType;
const clear = createDisposable(() => accessor = createSomething());
// vs
const [accessor, clear] = createBranch(dispose => [createSomething(), dispose]);

The latter is simpler for typescript since it allows inference, and also tuples are more familiar. However createDisposable is simpler for primitives that don't return an accessor. runWithSubRoot currently provides for both situations which avoids splitting the use case between two functions, but has the unfortunate downside of an inconsistent return value if you want to ignore the callback's return value, or a return value is sometimes undefined or null (note: this can be worked around https://playground.solidjs.com/?hash=-2033266374&version=1.3.13; this workaround also helps with backwards compatibility if we want to standardize return types for some packages).

[thetarnav]

For me the primitives should do as little as possible. All the reactive createEventListener should do is to react to changes to the reactive arguments. Being disposable should be out of it's concern. Besides how would the arguments be passed to createDisposable? – it allows the "owner dependencies" to be passed explicitly. Also most users don't care about disposing of event listener early, so adding that logic to the primitive would only bloat it for them. As for the "smart return" of the createDisposable, I'm also not convinced. It would be more flexible – yes. But also similarly confusing as the first iteration. I prefer the current option as it is the most simple.