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

(docs) Add tentative type annotations to Task methods #94

Closed
wants to merge 3 commits into from

Conversation

justin-calleja
Copy link
Contributor

  1. The constructor type annotations are in need of guidance.
  2. Special attention needs to be given to and and or type annotations.
    • and, got from here. Was going to go with: (Task a b).(Task c d) => (Task a _) or (Task c _) or (Task _ (b, d))) - why is the rejected part of both Tasks the same type a?

@justin-calleja
Copy link
Contributor Author

Also, from here, I could not figure out the difference between fat arrow (=>) and arrow (->).

@robotlolita
Copy link
Member

The function notation is (arg1, …, argN) => resultType, so only => arrows are used.

As for the type of or/and, the error type is the same for simplicity. While in JS you can use taskA.or(taskB) and have the error types in taskA and taskB be different, for simplicity we assume a more restricted system where logical type unions are less prevalent.

Ideally this'd encourage people to keep their types a bit more consistent, too, as something like (Task String Number).or(Task String Number) leads to simpler code (the type is always the same) than something like (Task String String).or(Task Error (Array Number)), where one'd have to use typeof and other checks, and write different branches for each case.

So, for that reason, we kinda arbitrarily restrict the types to be a bit more consistent:

or :: (Task a b).(Task a b) => Task a b
and :: (Task a b).(Task a c) => Task (a, c)

These types should also include the resources. Each task has an unique resource associated with it, which is consumed when you run that task (automatically by the TaskExecution). There's no real type notation for this concept yet (linear types), so they can be simplified by just putting the resource type on the LHS and RHS.

The constructor's type would be:

Task :: forall value, reason, resources:
  new ( ({ resolve: (value) => Void, reject: (reason) => Void, cancel: () => Void }) => resources
  , (resources) => Void
  , (resources) => Void
  ) => Task value reason resources 

And then each method transforming a task just keeps the same resource:

Task.map :: forall v1, v2, e, r: (Task v1 e r).((v1) => v2) => Task v1 e r

Methods combining tasks yield a combination of both resources:

Task.or :: forall v, e, r1, r2: (Task v e r1).(Task v e r2) => Task v e (r1 and r2)

@justin-calleja
Copy link
Contributor Author

@robotlolita thanks! The changes based on your feedback are WIP.

@justin-calleja
Copy link
Contributor Author

btw, re documenting Task based on Result (gitter discussion) - I have started with type annotations and looking at tests.

Next, I'll be looking at docs/source/en/data/result. If I'm comfortable with writing docs for Task based on that, I'll open a separate PR for that (that is what I think you were referring to in Gitter - but I didn't even know about the whole re-work of Task in Folktale 2... so need to get familiar before even giving it a shot). - mentioning this in case you have other priorities on what to work on ;)

@justin-calleja
Copy link
Contributor Author

@robotlolita

Shouldn't this:

and :: (Task a b).(Task a c) => Task (a, c)

be:

and :: (Task a b).(Task a c) => Task a (b, c)

?

@safareli
Copy link

and :: (Task a b).(Task a c) => Task a (b, c) is correct

@justin-calleja
Copy link
Contributor Author

Thanks @safareli !

@robotlolita
Copy link
Member

Ah, yes, sorry, my and type was incorrect :)

@justin-calleja
Copy link
Contributor Author

@robotlolita np

I was going to bring this up in my next push, but it might take a bit longer than expected so:

I noticed you've switched the rejected and resolved values in the type annotations.

i.e. data.Task was Task rejectedValue resolvedValue

but in your e.g. above it's Task v1 e r where I take it that v1 is resolved value, e is rejected value, and r is resources.

I've started the re-write of the type annotations using this new way (resolved value first) - as I figured, since Tasks are now created differently, perhaps it makes more sense to have the resolved values first.

Also, I personally like having resolution types first - but wanted to clear this up. In any case, it won't be a big deal to change again in case the above was just a typo.

Note, one thing that came to mind as a possible "inconsistency":

bimap takes the rejectionTransformation first which may be a bit misleading when the type of Task takes a resolution type as a first param (not a rejected one).

@justin-calleja
Copy link
Contributor Author

@robotlolita I'm not sure the changes I made are exactly what you had in mind.

Some things to look out for:

  1. Make sure I properly translated: "transforming of tasks keep the same resource" and "combining of tasks yield combination of both" in the changes.
  2. I assumed TaskExecution would also take the resources in its type so run returns a TaskExecution v e r.
  3. Again, as mentioned in previous comment above, the type of the resolved value now comes first.
  4. re of and rejected, there doesn't seem to be any resources in the type but I kept r there for consistency. Not sure if this is the right way to annotate those methods.

@robotlolita
Copy link
Member

That's pretty much it. In the places where there're two independent tasks, and the result is one of them, each task gets its own resource, and the resulting type has the resource of the task returned.

The resolved value part was an error on my part, sorry :< It doesn't really matter as long as the types line up, but as you mentioned it becomes inconsistent with things like bimap, fold and stuff that use more positional arguments.

@justin-calleja
Copy link
Contributor Author

@robotlolita np I'll make the changes re the resolved value position.

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

Successfully merging this pull request may close these issues.

3 participants