Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ObservableV2 template is too permissive #83

Open
djrenren opened this issue Nov 23, 2023 · 3 comments
Open

ObservableV2 template is too permissive #83

djrenren opened this issue Nov 23, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@djrenren
Copy link

djrenren commented Nov 23, 2023

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 djrenren added the bug Something isn't working label Nov 23, 2023
@djrenren
Copy link
Author

djrenren commented Nov 23, 2023

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
Copy link
Author

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
Copy link
Author

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) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants