search

TylorS / typed-unmaintained

The TypeScript Standard Library
MIT License
121 stars 7 forks source link

Fix curried chain Either type variant #76

Closed briancavalier closed 6 years ago

briancavalier commented 6 years ago

Fix the curried Chain type variant for Either, making it match the uncurried variant and correctly apply f to B instead of A.

I'd be happy to add a test that fails type checking, but I'm not quite sure why the existing tests passed. Perhaps defaulting B to any here is hiding type errors? What do you think?

Frikki commented 6 years ago

Thanks for the PR, @briancavalier. In my opinion, tests are always beneficial (if they test the right stuff.) And yes, that any default type shouldn’t be there. Good catch.

briancavalier commented 6 years ago

My pleasure, @Frikki. I'm not quite sure the best thing to do with those anys in TS. In TS 3, unknown might be the right thing. The error type should be inferable from the context of the return value. I'm not sure if TS's inference can handle that, but I'll see what happens.

briancavalier commented 6 years ago

I'm trying to get this line to fail.

If I undo this fix in this PR, and change the type of Either.of to:

export const of: <A>(value: A) => Either<never, A> = Right.of

yarn test still runs just fine with no type check errors, and I don't understand why. I'm having trouble imagining how TS is able to unify number (the right value type) with never (the left value type). It really seems like TS is confused:

1 2

Any ideas on how to proceed?

TylorS commented 6 years ago

Hey @briancavalier Thanks for the PR! I'll be able to take a better look at this later tonight.

One short-term goal of Typed is to upgrade to TS 3.x. (+ additions). I think using unknown would be a nice addition here

TylorS commented 6 years ago

After some discussion we've decided to go ahead and merge this fix, since we know it's correct, and will prioritize upgrading to 3.x in the near future.

Thanks @briancavalier for your PR!