Allow fn passed to Async.fromPromise to be partially applied

[JamieDixon]

This is a first-pass at allowing the function passed to Async.fromPromise to be partially applied by the caller.

One of the reasons I can think for not allowing this is that fromPromise can always be wrapped in a function that takes additional parameters and allows fromPromise to handle the final parameter passed. This is what I've been doing so far for example:

const updateRecord = (id, body) =>
  Async.fromPromise(companyId =>
    db.collection(`entities/${companyId}/foo`).doc(id).update(body)
  );

The motivation behind this addition is to provide a more natural feeling API where functions passed to crocks operations are curried by default.

With this change the code above would look like:

const updateRecord =
  Async.fromPromise((id, body, companyId) =>
    db.collection(`entities/${companyId}/foo`).doc(id).update(body)
  );

I appreciate that a lot of thought has gone into the API design of crocks and if this change is misplaced or misguided, I'd appreciate the feedback.

Thank you for all the hard work on this library, it's a great contribution to the community and has made a huge difference to my FP journey in particular.

[dalefrancis88]

Havn't looked in great detail but first thing i see is you should update the docs, https://crocks.dev/docs/crocks/Async.html#frompromise

btw sorry if you were going to do this already, just being eager to help

[coveralls]

Coverage remained the same at 100.0% when pulling 8c2f04d63e4f84a37273da5c2884673c082965e3 on JamieDixon:fromPromise-curry into ac80421de0b8ddcb525cf35fa97c491f7dd0afea on evilsoft:master.

[dalefrancis88]

Also run npm run contributors:add at some point, it'll add you to the list of contributors

[evilsoft]

@JamieDixon I find this usage very interesting, although it does break the API. Every function on the type constructors return an instance of the type. of, empty,id, ask, etc.

I am pretty sure we want to maintain that contract. Think of them as specific ways to create an instance. So I do not think this is the right place for a function like this.

I know personally that this will be a MAJOR breaking change, as we at my day job have all kinds of axios and knex routines that take advantage of the current API. To update all of them is a pretty big ask. I do not know how prevalent Async use is in other orgs, but I know this would hurt us big time.

So that said I am wondering if we could provide a helper function that can "ship" with the Async folder, like we do for getProp for Maybe. Would you be willing to explore that option?

[JamieDixon]

@evilsoft Thanks for your feedback and for looking over this PR. I appreciate your time is valuable and am grateful for any time you spend on this.

I'm very interested in the idea of shipping this kind of functionality as a separate module with the Async folder and I'd like to explore that idea further.

For my own curiosity I have a couple of questions:

1. In terms of maintaining the contract that functions on the type constructor return an instance of the type (Like of, empty, id, ask), is fromPromise already an exception to this interface because it doesn't return a type instance, but rather a function that eventually resolves to a type instance once called?

2. Additionally (and this is something I wondered earlier too), if fromPromise isn't part of that same contract, could it be that we need to move it off Async and into the Async folder?

3. I'm really interested to hear more about your usage of fromPromise that would break as a result of the passed function being capable of partial application. With this modification, current code that calls the returned function from fromPromise is expected to behave as it always has. What could I learn from the ways you're using fromPromise that reveal these changes to be breaking?

[evilsoft]

In terms of maintaining the contract that functions on the type constructor return an instance of the type (Like of, empty, id, ask), is fromPromise already an exception to this interface because it doesn't return a type instance, but rather a function that eventually resolves to a type instance once called?

Oh snap. so sorry you are 💯 on this and totes invalidates my earlier comment. (that is what I get for trying to do two things at once :smh:).

Additionally (and this is something I wondered earlier too), if fromPromise isn't part of that same contract, could it be that we need to move it off Async and into the Async folder?

I will really need to think on this for a bit. There was a reason it was included on the Constructor, but it escapes me now.

I'm really interested to hear more about your usage of fromPromise that would break as a result of the passed function being capable of partial application. With this modification, current code that calls the returned function from fromPromise is expected to behave as it always has. What could I learn from the ways you're using fromPromise that reveal these changes to be breaking?

Well that breakage would have come from the fantasy world I created in my head in which the function returned an Async. So in the real world, your proposed change is just fine, very eloquent and provides a path for people that just want unary or n-ary interfaces.

So I would like to take this opportunity to redact my previous, fantasy world comment as well as apologize for this unnecessary noise on such a fine PR. I see no reason for this not go forward. I will give it a full 👀 when you get those doc changes in there!

Thank you for your time and energy in your contribution, very much appreciated for sure.

[JamieDixon]

@dalefrancis88 @evilsoft Thank you both for the great feedback. I feel like this PR really showcases what good communication looks like in a review.

I've updated the documentation to reflect the new changes however I'm not sure whether the type signature for fromPromise needs to be adjusted to match these changes. Could one of you help me out with that part if it needs changing please?

[dalefrancis88]

Thanks @JamieDixon, @evilsoft is great for teaching open communication, he's had an affect on the way i comment and work through PR's internally where I work

[evilsoft]

Was able to re-run the Travis build and get it to report! As we are all 💚 now. Gonna merge this into master. Great Work @JamieDixon! :thanks: again!

[evilsoft]

Sorry I forgot to comment on this:

I've updated the documentation to reflect the new changes however I'm not sure whether the type signature for fromPromise needs to be adjusted to match these changes. Could one of you help me out with that part if it needs changing please?

The signature was wrong before. The changes in this PR match up to how the signature was originally crafted.

We typically use (*) to represent n-ary of varying types. See the signatures for things like unary, nAry and partial. This was GTG as provided!