dmonad / lib0

Monorepo of isomorphic utility functions
MIT License
363 stars 65 forks source link

ObservableV2 template is too permissive #83

Open djrenren opened 11 months ago

djrenren commented 11 months ago

Describe the bug

The ObservableV2 class allows the specification of a type which defines the set of events and callbacks, but the template is overly permissive. Namely, the EVENT parameter can constrain the callbacks such that every callback must return a value. This possibility makes it very difficult to write well-typed abstractions over ObservableV2 (they must handle the case where EVENTS has been instantiated this way).

To Reproduce

Here's a small example of this in typescript:

function foo() {
    let x = new ObservableV2<{"hi": () => number}>();
    x.on('hi', () => {}); // ERROR HERE
}

Which results in the error:

Argument of type '() => void' is not assignable to parameter of type '() => number'.
  Type 'void' is not assignable to type 'number'.ts(2345)

Expected behavior ObservableV2 should always allow callbacks that return void.

Additional context I ran into this problem while trying to write a utility function called waitUntil that runs a test on all emitted events and resolves a promise when it successfully passes the test. The untyped form is basically:

const waitUntil = (observable, event, test) => Promise.new(resolve => {
    function cb(..args) {
        if (test(...args)) {
            observable.off(event, cb);
            resolve(args);
        }
    }

    observable.on(event, cb);
});

This lets me write nice little functions like:

const waitUntilConnected = (client) => 
    waitUntil(client, "update", status => (status === "connected"));

Unfortunately it's impossible (or at least beyond me) to make the function cb inside waitUntil typecheck because the observable could require that callbacks for this event return a value.

djrenren commented 11 months ago

We can remove the degree of freedom from the type by simply not allowing the EVENT variable to define return types. Instead, using a map from event name to a list of parameter types.

- * @template {{[key in keyof EVENTS]: function(...any):void}} EVENTS
+ * @template {{[key in keyof EVENTS]: any[]}} EVENTS
  */
 export class ObservableV2 {

and then on the methods:

-   * @param {EVENTS[NAME]} f
+   * @param {(...args: EVENTS[NAME]) => void} f
    */
   on (name, f) {

So now usage would look like:

let x = new ObservableV2<{"someEvent": [Foo, number, string]}>();

x.on('someEvent', (foo: Foo, num: number, str: string) => { /* ... */ });
djrenren commented 11 months ago

And just to close the loop, this change does allow me to write the waitUntil abstraction like so:

type EventTable = {[event: string]: any[]};

export const waitUntil = <
    V extends EventTable,
    E extends string & keyof V
>(
    o: ObservableV2<V>,
    e: E,
    t: (...args: V[E]) => boolean
): Promise<V[E]> => new Promise(resolve => {
    let cb: (...args: V[E]) => void;
    cb = (...args) => {
        if (t(...args)) {
            o.off(e, cb);
            resolve(args)
        }
    }
    o.on(e, cb);
});
djrenren commented 11 months ago

As shown above, the return type information in the EVENTS map should be ignored. The solution above removes them so there's no more extraneous information in the type, but we could just ignore the return type by changing the types on the methods:


-      * @param {EVENTS[NAME]} f
+      * @param {(...args: Parameters<EVENTS[NAME]>) => void} f
       */
  on (name, f) {