cujojs / most

Ultra-high performance reactive programming
MIT License
3.49k stars 231 forks source link

Wrong typescript type for $.ap() #547

Open vlinder opened 3 years ago

vlinder commented 3 years ago

https://github.com/cujojs/most/blob/bfed148f1bfd8bc06a972a2cb17599371d8da52f/type-definitions/most.d.ts#L75

Not sure how this can have survived for so long, but this type suggests that you should pass a stream of functions to f$.ap() when in fact you should pass a stream of values.

The typings below should be the correct ones.

    ap<
      B = A extends (x: infer B) => any ? B : never,
      C = A extends (x: any) => infer C ? C : never
    >(
      x$: Stream<B>
    ): Stream<C>;

This also works

    ap(
      x$: Stream<A extends (x: infer B) => any ? B : never>
    ): Stream<A extends (x: any) => infer C ? C : never>;

Should I make a PR?

briancavalier commented 3 years ago

Hey @vlinder 👋 . I think the existing types are as intended. Here's the implementation of ap.

Can you help me understand why you feel they should be different?

vlinder commented 3 years ago

Thanks for your response!

The docs state that you should pass a stream of values. Or rather that you call .ap() on the stream of functions.

https://github.com/cujojs/most/blob/bfed148f1bfd8bc06a972a2cb17599371d8da52f/docs/api.md#L701

However the typings as I linked in the OP https://github.com/cujojs/most/blob/bfed148f1bfd8bc06a972a2cb17599371d8da52f/type-definitions/most.d.ts#L75 states that the passed stream should be a stream of functions.

I wrote a short program to verify which was right, the docs or the types. Typescript playground here

import { periodic } from "most";
const add = (a: number, b: number) => a + b;
const value$ = periodic(1000).constant(1).scan(add, 0);
const f$ = periodic(5000)
  .constant(1)
  .scan(add, 0)
  .map((x) => (y: number) => x * y);
f$.ap(value$).forEach(console.log);

image

This code work well, but it does not type check since the .ap function is typed incorrectly. In our repository we have a package dedicated to fixing the type issues in the most npm package that override some of the functions we need to have correct or better typings for (arbitrary amount of arguments mostly and this https://github.com/cujojs/most/pull/536). After adding my typings from OP it now both works and type checks.

I would gladly create a PR with those improved typings if there was a chance for them to be merged and released. But since my last PR that was very simple was never responded to I'm a bit hesitant to put in the effort if it is just going to be ignored. Let me know! :)