-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add a TypeScript definition file #129
Conversation
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 48 48
Lines 1103 1103
=====================================
Hits 1103 1103 Continue to review full report at Codecov.
|
package.json
Outdated
@@ -3,6 +3,7 @@ | |||
"version": "6.2.8", | |||
"description": "FantasyLand compliant (monadic) alternative to Promises", | |||
"main": "index.js", | |||
"typings": "index.d.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that "typings"
is an alias for "types"
. Let's use "types"
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidchambers Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is my very first experience with the typescript system. I know little about its syntax or restrictions. Most of the comments below are aimed at exploring the boundaries of the type system, and hopefully improve our typings file in the process.
index.d.ts
Outdated
interface FunctionReturning<T extends Function> { | ||
(...t: any[]): T | ||
} | ||
abstract class Future<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define Future<L, R>
instead, in order to define restrictions for the "error" branch?
index.d.ts
Outdated
(...t: any[]): T | ||
} | ||
abstract class Future<T> { | ||
done<T> (fn: (err: Error, value: T) => void): Future<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We better specify
fork
overdone
:done
is really just an alias. - I'm not sure, but this seems to state that
done
returns a Future, instead it returnsCancel
. Maybe we can define a type forCancel
, as is done in the README.
index.d.ts
Outdated
done<T> (fn: (err: Error, value: T) => void): Future<T> | ||
} | ||
|
||
function after<T> (duration: number, value: string): Future<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to define a PositiveInteger
and use that to restrict the input for duration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think thats possible using just typescript.
index.d.ts
Outdated
|
||
function after<T> (duration: number, value: string): Future<T> | ||
|
||
function and<A, B> (a: Future<A>, b: Future<B>): Future<A | B> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type can be specialized to be Future<B>
(the resolution value of a
is always ignored), when we introduce the error branch I think we can further improve the signature to:
function and<L, R> (a: Future<L, any>, b: Future<L, R>): Future<L, R>
index.d.ts
Outdated
|
||
function of<T> (value: T): Future<T> | ||
|
||
function or<A, B> (a: Future<A>, b: Future<B>): Future<A | B> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it's technically possible to provide two futures of different types to this function, I consider this a flaw in JavaScript due to the lack of its type system. Now that we have a type system, I think we should guide users to use consistent types. Therefore, I would like to see this signature change to something like:
function or<L, R> (a: Future<L, R>, b: Future<L, R>): Future<L, R>
index.d.ts
Outdated
// TODO: Need Help | ||
function par<T> (): Future<T> | ||
|
||
function parallel<T> (concurrency: number, futures: Array<Future<any>>): Future<Array<T>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This any
there could be T
, right?
Thank you for the effort thus far @tusharmath! I'm excited about providing this feature to users. Many of my colleagues rely on TypeScript for autocompletions and other goodies, and I would be delighted if these are made available for Fluture! |
index.d.ts
Outdated
abstract class Future<T, S> { | ||
done<T, S> (fn: (err: S, value: T) => void): Future<T, S> | ||
|
||
fork (reject: RejectFunction<S>, resolve: ResolveFunction<T>): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fork
returns Cancel
index.d.ts
Outdated
(...t: any[]): T | ||
} | ||
abstract class Future<T, S> { | ||
done<T, S> (fn: (err: S, value: T) => void): Future<T, S> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
returns cancel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we are only going to add typings for fork
?
#129 (comment)
index.d.ts
Outdated
done<T, S> (fn: (err: S, value: T) => void): Future<T, S> | ||
|
||
fork (reject: RejectFunction<S>, resolve: ResolveFunction<T>): void | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fluture has many more methods than just fork
and done
. Almost every function in Fluture is also available as a method. We should provide typings for these as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be once we are done with the static functions we could update the methods also?
index.d.ts
Outdated
} | ||
|
||
interface FunctionReturning<T> { | ||
(...t: any[]): T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fluture has no variadic functions in its code-base. I think we should be able to avoid using this type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is an internal type I can make it private to this module.
index.d.ts
Outdated
fork (reject: RejectFunction<S>, resolve: ResolveFunction<T>): void | ||
} | ||
|
||
function after<T, S> (duration: number, value: string): Future<T, S> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define a PositiveInteger
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think thats possible in Typescript as of today.
index.d.ts
Outdated
interface FunctionReturning<T> { | ||
(...t: any[]): T | ||
} | ||
abstract class Future<T, S> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that the positions of the generic types correspond to the positions of the functions in fork
. In my example I named them L
and R
to mean "left" and "right" respectively. We can keep the names S
and T
, if that's a convention of sorts, but I would prefer the generic on the left to represent the type of the rejection reason, and the generic on the right to represent the resolution value.
index.d.ts
Outdated
|
||
function after<T, S> (duration: number, value: string): Future<T, S> | ||
|
||
function and<A, B> (a: Future<A, any>, b: Future<B, any>): Future<A | B, any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to avoid mixed types, and force users to use consistent types. We can avoid a mixed type in this function by making the input more strict:
function and<L, R> (a: Future<L, any>, b: Future<L, R>): Future<L, R>
index.d.ts
Outdated
// TODO: Need Help | ||
function ap<T, S> (): Future<T, S> | ||
|
||
function attempt<T, S> (f: () => T): Future<T, S> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible in TypeScript to capture the type of the values that may be thrown from a function, similarly to how Java has void fn() throws TypeA, TypeB {...}
?
index.d.ts
Outdated
|
||
function of<T, S> (value: T): Future<T, S> | ||
|
||
function or<A, B> (a: Future<A, any>, b: Future<B, any>): Future<A | B, any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment under and
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the overwhelming amount of comments. Your efforts are a great help; I would not be able to implement TypeScript from scratch on my own. You're giving me a great practical introduction and starting point for typings in Fluture. If you're tired of working on this let me know and I might try to take over.
index.d.ts
Outdated
@@ -0,0 +1,133 @@ | |||
declare module 'fluture' { | |||
interface ResolveFunction<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we rename the generic to R
for consistency?
index.d.ts
Outdated
interface ResolveFunction<T> { | ||
(value: T): void | ||
} | ||
interface RejectFunction<S> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we rename the generic to L
for consistency?
index.d.ts
Outdated
fork (reject: RejectFunction<L>, resolve: ResolveFunction<R>): Cancel | ||
} | ||
|
||
export function after<L, R> (duration: number, value: string): IFuture<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value
should be of type R
.
index.d.ts
Outdated
|
||
export function after<L, R> (duration: number, value: string): IFuture<L, R> | ||
|
||
export function and<L, R> (a: IFuture<L, R>, b: IFuture<L, R>): IFuture<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can relax this signature a little bit by using allowing a
to be a IFuture<L, any>
, since the R
of a
is always ignored. Let's also rename a
to left
and b
to right
.
index.d.ts
Outdated
export function bimap<A, B, AA, BB> (left: (a: A) => AA, | ||
right: (a: B) => BB): IFuture<AA, any> | ||
|
||
export function both<A, B> (a: IFuture<A, any>, b: IFuture<B, any>): IFuture<[A, B], any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one needs to be updated to use the swapped positions of the generics:
export function both<L, A, B> (left: IFuture<L, A>, right: IFuture<L, B>): IFuture<L, [A, B]>
index.d.ts
Outdated
|
||
export function parallel<L, R> (concurrency: number, futures: Array<IFuture<any, any>>): IFuture<Array<L>, any> | ||
|
||
export function promise<L> (f: IFuture<L, any>): Promise<L> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where the Promise type is coming from, or why it only has one generic, but let's swap our own generics:
export function promise<R> (f: IFuture<any, R>): Promise<R>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promises are available globally out of the box if you are using typescript in your project.
index.d.ts
Outdated
|
||
export function promise<L> (f: IFuture<L, any>): Promise<L> | ||
|
||
export function race<A, B> (a: IFuture<A, any>, b: IFuture<B, any>): IFuture<A | B, any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's force users not to mix their types:
export function race<L, R> (left: IFuture<L, R>, right: IFuture<L, R>): IFuture<L, R>
index.d.ts
Outdated
// TODO: Need Help | ||
export function value<L, R> (): IFuture<L, R> | ||
|
||
export function Future<L, R> (fn: (reject: RejectFunction<L>, resolve: ResolveFunction<R>) => Cancel): IFuture<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename fn
to computation
.
index.d.ts
Outdated
export function encase3<L, R, A, B, C> (fn: (a: A, b: B, c: C) => L, a: A, b: B, c: C): IFuture<L, R> | ||
|
||
export function encase<L, R> (fn: FunctionReturning<L>, ...t: any[]): IFuture<L, R> | ||
export function encase<L, R> (fn: FunctionReturning<L>): {(...t: any[]): IFuture<L, R>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use FunctionReturning<T>
here, and (...) => T
in the other arities of encase
? I would prefer the latter for all arities, and removing the FunctionReturning
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I realized this earlier today :)
does the following make sense —
export function encase<L, R> (fn: () => R): {(): IFuture<L, R>}
export function encase<L, R, V> (fn: (v: V) => R, t: V): IFuture<L, R>
export function encase<L, R, V> (fn: (v: V) => R): {(t: V): IFuture<L, R>}
Apparently encase
is polymorphic so we should support all the three cases.
index.d.ts
Outdated
// TODO: Need Help | ||
export function hook<L, R> (): IFuture<L, R> | ||
|
||
export function isFuture<L, R> (fut: any): boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename fut
to value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen some overloading which I think exists to support partial function application. If we want to be thorough, we'd have to add these overloaded function definitions for every function. Is there a better way to express curried functions?
index.d.ts
Outdated
): IFuture<L, R> | ||
|
||
export function ap<L, R, VR>( | ||
apply: IFuture<L, (value: VR) => R>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I suggested (A) => B
is because it's clearer that we're transforming a value. I think (VR) => R
appears to imply that we're removing a layer (the V
) somehow, perhaps by unwrapping something. You made the point that A
/B
goes against the L
/R
convention we've been maintaining, so what do you think about RA
/RB
?
Furthermore, the ap
function (as opposed to the method) actually works on any Apply
. Ideally we express this through TypeScript, but I'm not sure if that's possible. A lot of people have looked into that though, I believe @gcanti has, with his gcanti/fp-ts project, gotten quite far with that.
If we can do this, then we could probably reuse this version of the signature in the definition of IFuture#ap
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ap function (as opposed to the method) actually works on any Apply
Now I'm curious, what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have a look at the documentation for ap
. When you expand the function signature, we see this:
ap :: Apply m => m (a -> b) -> m a -> m b
Future.ap :: Apply m => m (a -> b) -> m a -> m b
Future.prototype.ap :: Future e (a -> b) ~> Future e a -> Future e b
This shows us that there are three distinct versions of the ap
function. The first and the second are the same, so we can ignore that. It just serves to show that the function lives on the Future
class (for Static Land compliance) as well as as a named export.
Let's compare the first and the third.
The first is the named export. It's a function with signature:
ap :: m (a -> b) -> m a -> m b
Or in TypeScript (I think):
function ap<T, A, B>(T<(A) => B>, T<A>): T<B>
Except that the Hindley-Milner signature has one more part, the leading Apply m =>
. This indicates that, though m
is a generic type, it must conform to the Apply
interface (as defined by Fantasy Land). I don't know exactly how to translate that bit into TypeScript.
The third is the ap
method. It lives on the prototype of the Future class and is essentially ap
, but specialized for the Future type:
ap :: Future e (a -> b) ~> Future e a -> Future e b
The m
has been replaced with Future e
, and the Apply m =>
restriction can therefore be omitted. I find it difficult to translate this to TypeScript, let me illustrate:
class Future<L, R> {
ap<A, B>(value: Future<L, A>): Future<L, B>
}
This TypeScript definition is pretty crummy, as it fails to show where A
goes, and where B
comes from. The fact being that R
in this case must be (A) => B
. If we would want to improve it, we would have to make use of interfaces such as is done here, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it, thanks. So Future.ap
is not specialised for the Future type (and it leverages Z.ap
I guess)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be to specialise Future.ap
too, since using a particular encoding for HKTs would couple this library with that implementation.
Writing the bindings of Fluture
for fp-ts
is actually pretty easy and could be done in userland
//
// fp-ts bindings for Fluture
//
import { ap, of, map, chain, IFuture } from 'fluture'
import { Monad } from 'fp-ts/lib/Monad'
declare module 'fp-ts/lib/HKT' {
interface URI2HKT2<L, A> {
Fluture: IFuture<L, A>
}
}
declare module 'fluture' {
interface IFuture<L, R> {
_A: R
_L: L
_URI: URI
}
}
export const URI = 'Fluture'
export type URI = typeof URI
export const monad: Monad<URI> = {
URI,
map,
of,
ap,
chain
}
//
// usage
//
import { lift } from 'fp-ts/lib/Functor'
const double = (n: number) => n * 2
// liftedDouble: <L>(fa: IFuture<L, number>) => IFuture<L, number>
export const liftedDouble = lift(monad, double)
index.d.ts
Outdated
export function bimap<A, B, AA, BB>( | ||
left: (a: A) => AA, | ||
right: (a: B) => BB | ||
): IFuture<AA, any> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use LA
, LB
, RA
, and RB
. I think it's also nice to use names that correspond to the variable names used in Fluture's source-code. Furthermore, this function is missing its third parameter.
bimap<LA, LB, RA, RB>(
lmapper: (reason: LA) => LB),
rmapper: (value: RA) => RB),
source: IFuture<LA, RA>
): IFuture<LB, RB>
index.d.ts
Outdated
|
||
export function cache<L, R>(source: IFuture<L, R>): IFuture<L, R> | ||
|
||
export function chain<L, R, RR>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use RA
and RB
?
index.d.ts
Outdated
|
||
export function done<L, R>( | ||
fn: (cb: (err: L | null, value: R) => void) => void | ||
): IFuture<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you confused this function for Future.node
. Instead it be somewhat like this:
done<L, R>(
callback: (err: L | null, value: R?) => void,
source: IFuture<L, R>
): Cancel
index.d.ts
Outdated
): IFuture<L, R> | ||
|
||
export function encase2<L, R, A, B>( | ||
fn: (a: A, b: B) => L, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of the function should be R
, not L
.
index.d.ts
Outdated
|
||
export function of<L, R>(value: L): IFuture<L, R> | ||
|
||
export function or<L, R>(a: IFuture<L, R>, b: IFuture<L, R>): IFuture<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use left
and right
as parameter names.
index.d.ts
Outdated
export function parallel<L, R>( | ||
concurrency: number, | ||
futures: Array<IFuture<any, any>> | ||
): IFuture<L, Array<any>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really prefer to force developers to use consistent types. Again, the fact that it's possible in Fluture is simply because it has a huge performance impact to impose this restriction at run-time.
Like making multiple HTTP requests all of them returning different json responses
I would like for Fluture to move developers to normalize the different responses before passing the futures into parallel
. Futures that resolve to different types of values should have never made it into the same list.
The same goes for the rejection branch. Once you end up with a Future of a mixed type in your rejection branch, you're going to have a bad time handling that in fork
. My preference is still for changing this signature to:
parallel<L, R>(
concurrency: number,
futures: Array<IFuture<L, R>>
): IFuture<L, Array<R>>
index.d.ts
Outdated
futures: Array<IFuture<any, any>> | ||
): IFuture<L, Array<any>> | ||
|
||
export function promise<R>(f: IFuture<any, R>): Promise<R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename f
to source
.
index.d.ts
Outdated
(value: R): void | ||
} | ||
interface RejectFunction<L> { | ||
(error: L): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename error
to reason
.
index.d.ts
Outdated
right: IFuture<L, R> | ||
): IFuture<L, R> | ||
|
||
export function reject<L, R>(value: L): IFuture<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename value
to reason
.
@Avaq I guess because of different time zones the to and fro takes long. To save time I have added you as a collaborator to my fork. |
index.d.ts
Outdated
fork(reject: RejectFunction<L>, resolve: ResolveFunction<R>): Cancel | ||
} | ||
|
||
export function after<L, R>(duration: number, value: R): IFuture<L, R> | ||
export function after<L, R>(duration: number, value: R): Future<L, R> | ||
export function after<L, R>(duration: number): ({(value: R): Future<L, R>}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: the form
export function after<L, R>(duration: number): (value: R) => Future<L, R>
is more idiomatic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please nitpick :)
That's a great improvement!
index.d.ts
Outdated
@@ -31,75 +31,75 @@ declare module 'fluture' { | |||
} | |||
|
|||
export function after<L, R>(duration: number, value: R): Future<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add some documentation here
/** Creates a Future which resolves with the given value after n milliseconds. */
export function after<L, R>(duration: number, value: R): Future<L, R>
export function after<L, R>(duration: number): (value: R) => Future<L, R>
will show up in the IDE (for example vscode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip. Will do! :)
Right. Onward to the methods:
|
@tusharmath I added all methods, all remaining functions, currying overloadings to all functions and documentation to all methods and functions. Let's wait for a review of @gcanti and then I'll ask you to rebase on |
index.d.ts
Outdated
export function encase3<L, R, A, B, C>(fn: (a: A, b: B, c: C) => R, a: A): (b: B) => (c: C) => Future<L, R> | ||
export function encase3<L, R, A, B, C>(fn: (a: A, b: B, c: C) => R): (a: A, b: B, c: C) => Future<L, R> | ||
export function encase3<L, R, A, B, C>(fn: (a: A, b: B, c: C) => R): (a: A) => (b: B, c: C) => Future<L, R> | ||
export function encase3<L, R, A, B, C>(fn: (a: A, b: B, c: C) => R): (a: A) => (b: B) => (c: C) => Future<L, R> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach to currying doesn't actually seem to work:
import {encase3} from 'fluture'
encase3((a, b, c) => a + b + c) /*curry*/ ('a', 'b', 'c')
^^^^^
Cannot use operator '+' on type '{}'
Or, in other words: TypeScript failed to infer that we are passing a String there, and +
should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, when annotating a:string, b:string, c:string
, TypeScript will let it pass.
Unfortunately there's another, bigger, problem. The return value of these functions is not overloaded. TypeScript considers the following two definitions a "conflict" because they both have the exact same types of arguments, but "differing" return values:
export function encase3<L, R, A, B, C>(fn: (a: A, b: B, c: C) => R): (a: A, b: B, c: C) => Future<L, R>
export function encase3<L, R, A, B, C>(fn: (a: A, b: B, c: C) => R): (a: A) => (b: B, c: C) => Future<L, R>
And it deals with it by simply ignoring the second.
I'm going on a two weeks vacation, starting in an hour. I'm fairly sure I won't be working on anything during this period, so unfortunately I will have to leave it at this. If any of you may feel like continuing this work, @tusharmath could add you as a contributor to his fork. :) I'll pick things back up once I have returned. |
I have been on a vacation my self :) |
I believe that all that's left to do is to rewrite all curried functions using the Awaiting interfaces, as shown in my last commit. :) |
6ae9c1f
to
d209e21
Compare
I completed the work, I rebased on |
I believe these typings will only work for the ES6 module version of Fluture, correct? We might consider adding a note about this to the README if so. Furthermore, @gcanti told me it would be easy for users to include overloadings for polymorphic functions such as Finally, I'm wondering whether there is some kind of automated testing of the typings that we can do, perhaps just linting it for pitfalls. Anyone know of any such tools? |
I'm using typings-checker for that, you can see some examples in typelevel-ts or io-ts |
FYI just tested this last version against fp-ts and seems to work pretty well //
// fp-ts bindings for Fluture (likely will end up in a package like fp-ts-rxjs https://github.com/gcanti/fp-ts-rxjs)
//
export const URI = 'Fluture'
export type URI = typeof URI
declare module 'fp-ts/lib/HKT' {
interface URI2HKT2<L, A> {
Fluture: Future<L, A>
}
}
declare module 'fluture' {
interface Future<L, R> {
_A: R
_L: L
_URI: URI
}
}
//
// Usage
//
import { map, of, ap, chain, Future, reject } from 'fluture'
import { Monad } from 'fp-ts/lib/Monad'
export const fluture: Monad<URI> = {
URI,
map,
of,
ap,
chain
}
import { sequence } from 'fp-ts/lib/Traversable'
import * as array from 'fp-ts/lib/Array'
sequence(fluture, array)([of(1), reject('ops')]).fork(() => console.error('error'), xs => console.log(xs)) // => "error"
sequence(fluture, array)([of(1), of(2)]).fork(() => console.error('error'), xs => console.log(xs)) // => [1, 2] |
Thanks for the reference! I also just found this sanctuary-js/sanctuary#254 (comment) |
Darn. It looks like our way of handling curried functions was yet again problematic: sanctuary-js/sanctuary#431 (comment) |
Right. I'm ready to merge this PR. Can you given it a last look @tusharmath, @davidchambers and @gcanti?
|
README.md
Outdated
@@ -58,7 +58,8 @@ getPackageName('package.json') | |||
|
|||
### In ES6 or newer environments | |||
|
|||
The `package.json` sets a `module`-field for build-tools like [Rollup][]. | |||
The `package.json` sets a `module`-field for build-tools like [Rollup][]. The | |||
module version also has Typescript definitions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Typescript/TypeScript/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, aside from the typo I just mentioned. :)
LGTM |
|
||
interface Generator<Y, R> { | ||
(): Iterator<Y, R> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. By the looks of that interface, Next<A>
and Done<B>
must be expressed as one IteratorResult<A|B>
. I don't like that because it means we would have to relax the type signature for chainRec
.
I've merged these changes. If you feel strongly about the Iterator interfaces, @tusharmath, feel free to open a separate issue or PR. :) |
@Avaq great, let me know when you release this so I can work on the fp-ts bindings package |
@Avaq This is great! |
I see there is a never type here. Typescript has a never type according to here Is this a conflict? What's the purpose of this fluture''s never type? I see it's used as the left type parameter on the return type of |
As I understand, TypeScript allows types and values to share names, and the meaning of an identifier will change depending on context. Fluture does not export a The type of the
You'll have to expand on that a little. When does it complain? If you're trying to use the |
@Avaq
I have created the PR but could use some help since there are a lot utility functions to give typings for.
TODO
after()
and()
ap()
attempt()
bimap()
both()
cache()
chain()
done()
encase()
encase2()
encase3()
encaseN()
encaseN2()
encaseN3()
encaseP()
encaseP2()
encaseP3()
extractLeft()
extractRight()
fold()
fork()
go()
hook()
isFuture()
isNever()
lastly()
map()
mapRej()
never()
node()
of()
or()
par()
parallel()
promise()
race()
reject()
rejectAfter()
seq()
swap()
tryP()
value()
Future()