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

Extra Operator Generics in Typescript 2.4 #202

Closed
WorldMaker opened this issue Jun 30, 2017 · 3 comments
Closed

Extra Operator Generics in Typescript 2.4 #202

WorldMaker opened this issue Jun 30, 2017 · 3 comments

Comments

@WorldMaker
Copy link
Contributor

WorldMaker commented Jun 30, 2017

Typescript 2.4 has better generic type inference, including better support for unifying return types in generic functions with the generic type parameter. I'm getting type inferencing issues in Typescript 2.4 that I can't seem to solve (x is not x errors) trying to use some of the extra operators and I think it's because of the way the generics are used in the library.

Take the signature for dropRepeats for example:

export default function dropRepeats(isEqual: (<T>(x: T, y: T) => boolean) | undefined = void 0): <T>(ins: Stream<T>) => Stream<T>

In Typescript 2.4 I'm getting an error because the generic parameter T for the isEqual is not the generic parameter T for the return function. That's not something that Typescript prior to 2.4 has checked.

It seems to me that the obvious solution to move the generic parameter to the outer function would both fix this problem for Typescript 2.4 and happen to give better type inferencing in Typescript before 2.4:

export default function dropRepeats<T>(isEqual: ((x: T, y: T) => boolean) | undefined = void 0): (ins: Stream<T>) => Stream<T> {
  return function dropRepeatsOperator(ins: Stream<T>): Stream<T> {
    return new Stream<T>(new DropRepeatsOperator<T>(ins, isEqual));
  };
}

This updated version of the operator function passes all the existing tests in Typescript 2.1 (current package.json version). The tests don't currently run in Typescript 2.4; seems like there are updates need to be made to the mocha and other types first.

I'm happy to make a PR to move the generics in the extra operators to this simpler form, but I wanted to make sure I wasn't missing some reason for why the generics were written the way they were originally.

@staltz
Copy link
Owner

staltz commented Jul 12, 2017

Hi @WorldMaker.

I understand TS 2.4 has better inference, and we want to update xstream to use it. We should do that in a branch, release an rc version, and only release a real version at the same time as we release a real version for Cycle.js packages. We should do the same updating workflow in both xstream and cyclejs repositories.

I tried it out here, and the typing you suggested

function dropRepeats<T>(
  isEqual: ((x: T, y: T) => boolean) | undefined = void 0
): (ins: Stream<T>) => Stream<T> {
  return function dropRepeatsOperator<T>(ins: Stream<T>): Stream<T> {
    return new Stream<T>(new DropRepeatsOperator<T>(ins, isEqual));
  };
}

Does not compile with TypeScript v2.1.5, it reports:

src/extra/dropRepeats.ts(120,58): error TS2345: Argument of type '((x: T, y: T) => boolean) | undefined' is not assignable to parameter of type '((x: T, y: T) => boolean) | undefined'.
  Type '(x: T, y: T) => boolean' is not assignable to type '((x: T, y: T) => boolean) |undefined'.
    Type '(x: T, y: T) => boolean' is not assignable to type '(x: T, y: T) => boolean'.Two different types with this name exist, but they are unrelated.
      Types of parameters 'x' and 'x' are incompatible.
        Type 'T' is not assignable to type 'T'. Two different types with this name exist, but they are unrelated.

Since you get a similar error when using the current xstream codebase with TS v2.4, it seems TS v2.4 is a breaking change, that's why I'm proposing that rc-based update workflow.

Thanks for reporting.

@staltz
Copy link
Owner

staltz commented Jul 12, 2017

Ignore what I just said, I was missing a detail

 function dropRepeats<T>(
   isEqual: ((x: T, y: T) => boolean) | undefined = void 0
 ): (ins: Stream<T>) => Stream<T> {
-  return function dropRepeatsOperator<T>(ins: Stream<T>): Stream<T> {
+  return function dropRepeatsOperator(ins: Stream<T>): Stream<T> {
     return new Stream<T>(new DropRepeatsOperator<T>(ins, isEqual));
   };
 }

So yeah, we need to do this:

I'm happy to make a PR to move the generics in the extra operators to this simpler form, but I wanted to make sure I wasn't missing some reason for why the generics were written the way they were originally.

WorldMaker added a commit to WorldMaker/xstream that referenced this issue Jul 12, 2017
Move the generic parameters of the extra operators to support better generic parameter
inferencing in Typescript 2.4 while continuing to pass tests in Typescript 2.1. Delay is
the only extra operator of this form skipped in this commit because it is the only one
that didn't pass Typescript inferencing compile (in imitate tests) in Typescript 2.1
(though it builds in Typescript 2.4).

Resolves staltz#202
staltz pushed a commit that referenced this issue Jul 18, 2017
Move the generic parameters of the extra operators to support better generic parameter
inferencing in Typescript 2.4 while continuing to pass tests in Typescript 2.1. Delay is
the only extra operator of this form skipped in this commit because it is the only one
that didn't pass Typescript inferencing compile (in imitate tests) in Typescript 2.1
(though it builds in Typescript 2.4).

Resolves #202
staltz pushed a commit that referenced this issue Jul 18, 2017
Move the generic parameters of the extra operators to support better generic parameter
inferencing in Typescript 2.4 while continuing to pass tests in Typescript 2.1. Delay is
the only extra operator of this form skipped in this commit because it is the only one
that didn't pass Typescript inferencing compile (in imitate tests) in Typescript 2.1
(though it builds in Typescript 2.4).

Resolves #202
@staltz
Copy link
Owner

staltz commented Jul 18, 2017

PR #206 was merged to branch ts24 and we should release this soon as v11.0.0-rc.1.

@staltz staltz closed this as completed Jul 18, 2017
staltz pushed a commit that referenced this issue Sep 19, 2017
Move the generic parameters of the extra operators to support better generic parameter
inferencing in Typescript 2.4 while continuing to pass tests in Typescript 2.1. Delay is
the only extra operator of this form skipped in this commit because it is the only one
that didn't pass Typescript inferencing compile (in imitate tests) in Typescript 2.1
(though it builds in Typescript 2.4).

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

No branches or pull requests

2 participants