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

Wrong typescript type for $.ap() #547

Open
vlinder opened this issue Dec 15, 2020 · 2 comments
Open

Wrong typescript type for $.ap() #547

vlinder opened this issue Dec 15, 2020 · 2 comments

Comments

@vlinder
Copy link

vlinder commented Dec 15, 2020

ap<B, C>(fs: Stream<(a: A) => B>): Stream<C>;

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

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

vlinder commented Dec 17, 2020

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.

#### `streamOfFunctions.ap(stream) -> Stream`

However the typings as I linked in the OP

ap<B, C>(fs: Stream<(a: A) => B>): Stream<C>;
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 #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! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants