Adding promise-aware function composition. #524

Merged
merged 1 commit into from Nov 19, 2014

Conversation

Projects
None yet
9 participants
@yuri
Contributor

yuri commented Nov 15, 2014

Compose would now handle promises returned by any of the composed functions. The resulting function
will evaluate to a promise if any one of the composed functions returns a promise. If all of the composed functions return something other than a promise, then the behaviour of compose() is the same as it was before. In this sense the change is fully backwards compatible.

I added one new test and modified two tests to test promises. I added Q to dev dependencies to test promise support. However, Q is not necessary at run time, and this implementation should work with any promise implementation.

See #512 for discussion.

ramda.js
- return f.call(this, g.apply(this, arguments));
+ var context = this;
+ var value = g.apply(this, arguments);
+ if (value && value.then) {

This comment has been minimized.

@megawac

megawac Nov 15, 2014

Contributor

then should at least be a function
Also this is the kind of type juggling that is kinda against this libs beliefs

@megawac

megawac Nov 15, 2014

Contributor

then should at least be a function
Also this is the kind of type juggling that is kinda against this libs beliefs

@kedashoe

This comment has been minimized.

Show comment
Hide comment
@kedashoe

kedashoe Nov 15, 2014

Contributor

I really like this, but if I had a vote it would be to leave internal compose as it is and put this in a (newly created, with the idea that more things could be added) promise module.

Contributor

kedashoe commented Nov 15, 2014

I really like this, but if I had a vote it would be to leave internal compose as it is and put this in a (newly created, with the idea that more things could be added) promise module.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 15, 2014

Contributor

@kedashoe I don't think there is going to be much to add here. And I think promises are moving towards being a first class citizen in JS, so it makes sense to treat them as a part of the standard use case, rather than a special case.

@megawac Good point on checking that .then is a function. I'll update the PR.

Contributor

yuri commented Nov 15, 2014

@kedashoe I don't think there is going to be much to add here. And I think promises are moving towards being a first class citizen in JS, so it makes sense to treat them as a part of the standard use case, rather than a special case.

@megawac Good point on checking that .then is a function. I'll update the PR.

ramda.js
};
}
+ function isThennable(value) {
+ return value && value.then && typeof (value.then === 'function');

This comment has been minimized.

@buzzdecafe

buzzdecafe Nov 15, 2014

Member

typeof (value.then === 'function'); is boolean--the parentheses are getting in the way

@buzzdecafe

buzzdecafe Nov 15, 2014

Member

typeof (value.then === 'function'); is boolean--the parentheses are getting in the way

This comment has been minimized.

@yuri

yuri Nov 15, 2014

Contributor

Oops, misplaced parentheses.

@yuri

yuri Nov 15, 2014

Contributor

Oops, misplaced parentheses.

ramda.js
};
}
+ function isThennable(value) {
+ return value && value.then && typeof value.then === 'function';

This comment has been minimized.

@davidchambers

davidchambers Nov 16, 2014

Member

I'd prefer to use an explicit null/undefined guard:

return value != null && typeof value.then === 'function';
@davidchambers

davidchambers Nov 16, 2014

Member

I'd prefer to use an explicit null/undefined guard:

return value != null && typeof value.then === 'function';

This comment has been minimized.

@yuri

yuri Nov 16, 2014

Contributor

Ok, that's easily fixed. I'll update the PR.

@yuri

yuri Nov 16, 2014

Contributor

Ok, that's easily fixed. I'll update the PR.

@davidchambers

This comment has been minimized.

Show comment
Hide comment
@davidchambers

davidchambers Nov 16, 2014

Member

@megawac is right: Ramda generally avoids this sort of sniffing. I very much agree with the goal of this pull request, but I wish there were a less hacky way of detecting promises.

Member

davidchambers commented Nov 16, 2014

@megawac is right: Ramda generally avoids this sort of sniffing. I very much agree with the goal of this pull request, but I wish there were a less hacky way of detecting promises.

@fyyyyy

This comment has been minimized.

Show comment
Hide comment
@fyyyyy

fyyyyy Nov 16, 2014

Contributor

Hmm indeed. Would this break if my then method isn't a promise ?
Could the promises be wrapped explicitly as in R.compose(fn1, R.promise(fn2), fn3)

Contributor

fyyyyy commented Nov 16, 2014

Hmm indeed. Would this break if my then method isn't a promise ?
Could the promises be wrapped explicitly as in R.compose(fn1, R.promise(fn2), fn3)

@davidchambers

This comment has been minimized.

Show comment
Hide comment
@davidchambers

davidchambers Nov 16, 2014

Member

I like the idea of wrapping the promise-returning function, @fyyyyy, but I don't see how it would work. R.promise (or whatever we called it) would need to return a function which we know should return a promise. We could do something like this:

var promiseSentinel = {};

R.promise = function promise(fn) {
    var wrapper = arity(fn.length, function() {
        return fn.apply(this, arguments);
    });
    wrapper.ramdaPromise = promiseSentinel;
    return wrapper;
};

We could then test whether the value of function's ramdaPromise property is promiseSentinel. This seems rather unappealing, though.

Member

davidchambers commented Nov 16, 2014

I like the idea of wrapping the promise-returning function, @fyyyyy, but I don't see how it would work. R.promise (or whatever we called it) would need to return a function which we know should return a promise. We could do something like this:

var promiseSentinel = {};

R.promise = function promise(fn) {
    var wrapper = arity(fn.length, function() {
        return fn.apply(this, arguments);
    });
    wrapper.ramdaPromise = promiseSentinel;
    return wrapper;
};

We could then test whether the value of function's ramdaPromise property is promiseSentinel. This seems rather unappealing, though.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 16, 2014

Member

@davidchambers: I'm pretty sure that even by the spec, there is no better way to recognize promises.

(Update: This was not in response to your "sentinel" comment, but to the previous one.)

Member

CrossEye commented Nov 16, 2014

@davidchambers: I'm pretty sure that even by the spec, there is no better way to recognize promises.

(Update: This was not in response to your "sentinel" comment, but to the previous one.)

@fyyyyy

This comment has been minimized.

Show comment
Hide comment
@fyyyyy

fyyyyy Nov 16, 2014

Contributor

@davidchambers
Not sure either how this should work, but i like your idea neitherless. For one, it relieves implementation details from compose. If you need to support more types of promises in the future then this is the place to go to (probably?)

Probably i don't understand how compose works, so what i had in mind was that R.promise would keep it wrapped, and the user has to check himself if he wants the promise (via .then) or the composed function in the end. So R.compose would return a wrapper when R.promise was used somewhere. Probably silly 😄

Contributor

fyyyyy commented Nov 16, 2014

@davidchambers
Not sure either how this should work, but i like your idea neitherless. For one, it relieves implementation details from compose. If you need to support more types of promises in the future then this is the place to go to (probably?)

Probably i don't understand how compose works, so what i had in mind was that R.promise would keep it wrapped, and the user has to check himself if he wants the promise (via .then) or the composed function in the end. So R.compose would return a wrapper when R.promise was used somewhere. Probably silly 😄

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 16, 2014

Contributor

@davidchambers R.promise would add quite a bit of boilerplate, to the point where I think an externally supplied implementation of compose would be more attractive for someone who is looking to work with promises. If you feel that relying on existence of .then is too invasive, the better alternative would be to leave R.compose() as is and add a new promise-aware function R.pcompose(). But again, for better or worse, the spec's definition of thennables is: “thenable” is an object or function that defines a then method. So, the method that, say, Q, uses for detecting promises is this:

  function isPromiseAlike(object) {
      return isObject(object) && typeof object.then === "function";
  }

(I can switch to doing the same in my check.) AngularJS uses essentially the same method. So, code that makes use of objects with .then() that are not actually promises is going to have all sorts of trouble anyway.

@fyyyyy I don't think there are "more types of promises" to anticipate. There are different promise libraries around, but they all ultimately trade in "thennables", which is to say objects that carry a .then() method that can be used to attach callbacks. Nearly all promise libraries out there follow this approach, which is also becoming a part of ES6 standard. So, the solution that I am proposing is not tailored for a specific kind of promises. It's just makes the basic assumption that pretty much all promise libraries make.

Contributor

yuri commented Nov 16, 2014

@davidchambers R.promise would add quite a bit of boilerplate, to the point where I think an externally supplied implementation of compose would be more attractive for someone who is looking to work with promises. If you feel that relying on existence of .then is too invasive, the better alternative would be to leave R.compose() as is and add a new promise-aware function R.pcompose(). But again, for better or worse, the spec's definition of thennables is: “thenable” is an object or function that defines a then method. So, the method that, say, Q, uses for detecting promises is this:

  function isPromiseAlike(object) {
      return isObject(object) && typeof object.then === "function";
  }

(I can switch to doing the same in my check.) AngularJS uses essentially the same method. So, code that makes use of objects with .then() that are not actually promises is going to have all sorts of trouble anyway.

@fyyyyy I don't think there are "more types of promises" to anticipate. There are different promise libraries around, but they all ultimately trade in "thennables", which is to say objects that carry a .then() method that can be used to attach callbacks. Nearly all promise libraries out there follow this approach, which is also becoming a part of ES6 standard. So, the solution that I am proposing is not tailored for a specific kind of promises. It's just makes the basic assumption that pretty much all promise libraries make.

@fyyyyy

This comment has been minimized.

Show comment
Hide comment
@fyyyyy

fyyyyy Nov 17, 2014

Contributor

@yuri Guess you're right that promise people would switch to a different compose when they are forced to wrap every call.

Could one build it's own pCompose by doing var pCompose = R.promisable(R.compose), or R.promisable(R.pipe), or R.promisable(R.converge), and then use those throughout code? Otherwise just ignore my comment 😄 👍

Contributor

fyyyyy commented Nov 17, 2014

@yuri Guess you're right that promise people would switch to a different compose when they are forced to wrap every call.

Could one build it's own pCompose by doing var pCompose = R.promisable(R.compose), or R.promisable(R.pipe), or R.promisable(R.converge), and then use those throughout code? Otherwise just ignore my comment 😄 👍

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 17, 2014

Member

Although I definitely want this behavior, I'm still not sure whether I'd prefer to have a single compose that handles anything or one dedicated to handle the possibility of Promises as well as our standard one that simply deals with synchronous functions. What I'd really like is some performance benchmarks. If dealing with the possibilities of Promises slows down one of ours most important functions, compose, then we'd definitely have to do them separately.

Member

CrossEye commented Nov 17, 2014

Although I definitely want this behavior, I'm still not sure whether I'd prefer to have a single compose that handles anything or one dedicated to handle the possibility of Promises as well as our standard one that simply deals with synchronous functions. What I'd really like is some performance benchmarks. If dealing with the possibilities of Promises slows down one of ours most important functions, compose, then we'd definitely have to do them separately.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 17, 2014

Member

Although I definitely want this behavior, I'm still not sure whether I'd prefer to have a single compose that handles anything or one dedicated to handle the possibility of Promises as well as our standard one that simply deals with synchronous functions.

I also want the behavior, but IMO blending it in makes compose more difficult to reason about. and it's not clear how it would integrate with types, e.g. what is a Maybe of a (native-js-style) Promise?

Member

buzzdecafe commented Nov 17, 2014

Although I definitely want this behavior, I'm still not sure whether I'd prefer to have a single compose that handles anything or one dedicated to handle the possibility of Promises as well as our standard one that simply deals with synchronous functions.

I also want the behavior, but IMO blending it in makes compose more difficult to reason about. and it's not clear how it would integrate with types, e.g. what is a Maybe of a (native-js-style) Promise?

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

@CrossEye With a couple of optimizations, I get about 50% performance penalty, but only for composing absolutely most trivial functions, such as a(x) { return x+'A'; }, since in this case the trivial cost of running each composed function is comparable to the trivial cost of running the check presence of .then(). For functions that do non trivial stuff, however, the difference is smaller. And for trivial functions, we are talking about a difference of 100 ms over 1 million compositions.

Contributor

yuri commented Nov 17, 2014

@CrossEye With a couple of optimizations, I get about 50% performance penalty, but only for composing absolutely most trivial functions, such as a(x) { return x+'A'; }, since in this case the trivial cost of running each composed function is comparable to the trivial cost of running the check presence of .then(). For functions that do non trivial stuff, however, the difference is smaller. And for trivial functions, we are talking about a difference of 100 ms over 1 million compositions.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

@buzzdecafe So, would you prefer that I change the PR to make this a new function, R.pcompose() or R.composeAsync() or something similar?

Contributor

yuri commented Nov 17, 2014

@buzzdecafe So, would you prefer that I change the PR to make this a new function, R.pcompose() or R.composeAsync() or something similar?

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 17, 2014

Member

@yuri: That's probably not too bad for numbers. But I'm still not convinced that it belongs together with the standard compose.

Ramda generally does not like to combine multiple behaviors into a single function. We do make an exception for the dynamic dispatch of a number of functions, and this feels something like that, but I'm especially bothered by the same sorts of issues @buzzdecafe raises. I'm also bothered by the almost-but-not-quite-Functor style of Promises. But there's really nothing we can do about that.

One thing is certain. I want to add a dynamically dispatched then function to Ramda. There are too many possible uses for it to not do so.

Member

CrossEye commented Nov 17, 2014

@yuri: That's probably not too bad for numbers. But I'm still not convinced that it belongs together with the standard compose.

Ramda generally does not like to combine multiple behaviors into a single function. We do make an exception for the dynamic dispatch of a number of functions, and this feels something like that, but I'm especially bothered by the same sorts of issues @buzzdecafe raises. I'm also bothered by the almost-but-not-quite-Functor style of Promises. But there's really nothing we can do about that.

One thing is certain. I want to add a dynamically dispatched then function to Ramda. There are too many possible uses for it to not do so.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 17, 2014

Member

@yuri, I'd like to hear from others, but that's the way I'm leaning. pcompose or pchain or something like that makes sense to me.

Member

CrossEye commented Nov 17, 2014

@yuri, I'd like to hear from others, but that's the way I'm leaning. pcompose or pchain or something like that makes sense to me.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 17, 2014

Member

yes, the "it's almost a functor" thing is tantalizing but frustrating. put me down for 👍 for pcompose

Member

buzzdecafe commented Nov 17, 2014

yes, the "it's almost a functor" thing is tantalizing but frustrating. put me down for 👍 for pcompose

@kedashoe

This comment has been minimized.

Show comment
Hide comment
@kedashoe

kedashoe Nov 17, 2014

Contributor

put me down for 👍 for pcompose

👍

Contributor

kedashoe commented Nov 17, 2014

put me down for 👍 for pcompose

👍

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

Sounds good, pcompose it is. I'll update the PR.

Contributor

yuri commented Nov 17, 2014

Sounds good, pcompose it is. I'll update the PR.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

@CrossEye @buzzdecafe I updated the PR but pcompose() examples require a promise library and I couldn't get exampleRunner to include Q for some reason. Can you suggest a way of doing that?

Contributor

yuri commented Nov 17, 2014

@CrossEye @buzzdecafe I updated the PR but pcompose() examples require a promise library and I couldn't get exampleRunner to include Q for some reason. Can you suggest a way of doing that?

@fyyyyy

This comment has been minimized.

Show comment
Hide comment
@fyyyyy

fyyyyy Nov 17, 2014

Contributor

interesting
https://andreypopp.com/posts/2014-07-21-fighting-node-callbacks-with-purescript.html

This finally enables do-notation for computations on thunks, so we can write them with synchronous-looking code:

download url filename = do
contents <- getURL url
writeFile filename contents

Contributor

fyyyyy commented Nov 17, 2014

interesting
https://andreypopp.com/posts/2014-07-21-fighting-node-callbacks-with-purescript.html

This finally enables do-notation for computations on thunks, so we can write them with synchronous-looking code:

download url filename = do
contents <- getURL url
writeFile filename contents

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

@fyyyyy You can do this with ES6 generators, though. If it's going to require transpiling, might as well transpile ES6.

Contributor

yuri commented Nov 17, 2014

@fyyyyy You can do this with ES6 generators, though. If it's going to require transpiling, might as well transpile ES6.

@fyyyyy

This comment has been minimized.

Show comment
Hide comment
@fyyyyy

fyyyyy Nov 17, 2014

Contributor

@yuri thx i will check it out

Contributor

fyyyyy commented Nov 17, 2014

@yuri thx i will check it out

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 17, 2014

Member

@fyyyyy, @yuri: This could also be done with sweet.js or in various other transpiling techniques. But none of that helps us much with our current problem.

@yuri, I will try to look into this tonight. Have you actually included Q somewhere? At a quick glance, I just saw a reference in the test without an addition of Q itself. But I might have missed it.

Member

CrossEye commented Nov 17, 2014

@fyyyyy, @yuri: This could also be done with sweet.js or in various other transpiling techniques. But none of that helps us much with our current problem.

@yuri, I will try to look into this tonight. Have you actually included Q somewhere? At a quick glance, I just saw a reference in the test without an addition of Q itself. But I might have missed it.

@kedashoe

This comment has been minimized.

Show comment
Hide comment
@kedashoe

kedashoe Nov 17, 2014

Contributor

I updated the PR but pcompose() examples require a promise library and I couldn't get exampleRunner to include Q for some reason. Can you suggest a way of doing that?

I submitted #530 so you can do this. I tested by adding

var Q = require('q');

to the beginning of your example for _pcompose and everything seemed to go smoothly. However, worth noting the example runner is not promise aware (or more generally, asynchronous aware), so it won't actually verify any // => statements inside of a callback (I still like your example and think it is worth including Q to show what this does).

@contributors: PR only submitted as it is a separate issue from promise composition and wasn't sure how you felt about it being included as part of this. If you don't mind, I do not mind ignoring my PR and simply having @yuri add the 1 line in example runner to his commit.

Contributor

kedashoe commented Nov 17, 2014

I updated the PR but pcompose() examples require a promise library and I couldn't get exampleRunner to include Q for some reason. Can you suggest a way of doing that?

I submitted #530 so you can do this. I tested by adding

var Q = require('q');

to the beginning of your example for _pcompose and everything seemed to go smoothly. However, worth noting the example runner is not promise aware (or more generally, asynchronous aware), so it won't actually verify any // => statements inside of a callback (I still like your example and think it is worth including Q to show what this does).

@contributors: PR only submitted as it is a separate issue from promise composition and wasn't sure how you felt about it being included as part of this. If you don't mind, I do not mind ignoring my PR and simply having @yuri add the 1 line in example runner to his commit.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

@kedashoe Thanks. And damn it, I was so close! I'll add this one line to my PR.

Contributor

yuri commented Nov 17, 2014

@kedashoe Thanks. And damn it, I was so close! I'll add this one line to my PR.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

The examples work now. I think this is good to go now. @CrossEye @davidchambers @buzzdecafe What do you think?

Contributor

yuri commented Nov 17, 2014

The examples work now. I think this is good to go now. @CrossEye @davidchambers @buzzdecafe What do you think?

ramda.js
+ */
+ function isThennable(value) {
+ return (value != null) && (value === Object(value)) && value.then &&
+ typeof value.then === 'function';

This comment has been minimized.

@buzzdecafe

buzzdecafe Nov 17, 2014

Member

can't this be simplified to return value != null && typeof value.then === 'function';?

@buzzdecafe

buzzdecafe Nov 17, 2014

Member

can't this be simplified to return value != null && typeof value.then === 'function';?

This comment has been minimized.

@yuri

yuri Nov 17, 2014

Contributor

It can be. The reason for adding the test of being an object is that this speeds things up when working with numbers and strings. This is also the exact test that Q is using to test for thenness. But I can drop it.

@yuri

yuri Nov 17, 2014

Contributor

It can be. The reason for adding the test of being an object is that this speeds things up when working with numbers and strings. This is also the exact test that Q is using to test for thenness. But I can drop it.

This comment has been minimized.

@buzzdecafe

buzzdecafe Nov 17, 2014

Member

thanks for the explanation, i just wanted to understand the rationale for the extra tests.

@buzzdecafe

buzzdecafe Nov 17, 2014

Member

thanks for the explanation, i just wanted to understand the rationale for the extra tests.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 17, 2014

Member

apart from question on isThennable, this looks good to me

Member

buzzdecafe commented Nov 17, 2014

apart from question on isThennable, this looks good to me

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 17, 2014

Contributor

@CrossEye: Q is not required for pcompile() to work, so it's not a ramda dependency. The idea is that promises will come from functions provided by the user, and will be generated by whichever library those functions are using. I am using Q in examples and in tests, though, so I added it as a dev dependency. The test.compose.js and the examples have var Q = require('q'); in them.

Contributor

yuri commented Nov 17, 2014

@CrossEye: Q is not required for pcompile() to work, so it's not a ramda dependency. The idea is that promises will come from functions provided by the user, and will be generated by whichever library those functions are using. I am using Q in examples and in tests, though, so I added it as a dev dependency. The test.compose.js and the examples have var Q = require('q'); in them.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 17, 2014

Member

@yuri Yes, somehow, I didn't originally see the change to package.json. So I couldn't figure out how you were expecting these to work at all. Sorry about that.

Member

CrossEye commented Nov 17, 2014

@yuri Yes, somehow, I didn't originally see the change to package.json. So I couldn't figure out how you were expecting these to work at all. Sorry about that.

@fyyyyy

This comment has been minimized.

Show comment
Hide comment
@fyyyyy

fyyyyy Nov 18, 2014

Contributor

Just read this in source, would it make sense if thennable dispatches to R's internal

    function _hasMethod(methodName, obj) {
        return obj != null && !_isArray(obj) && typeof obj[methodName] === 'function';
    }
Contributor

fyyyyy commented Nov 18, 2014

Just read this in source, would it make sense if thennable dispatches to R's internal

    function _hasMethod(methodName, obj) {
        return obj != null && !_isArray(obj) && typeof obj[methodName] === 'function';
    }
@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 18, 2014

Contributor

@fyyyyy I think this would make sense. I'll update the PR if nobody objects to this.

Contributor

yuri commented Nov 18, 2014

@fyyyyy I think this would make sense. I'll update the PR if nobody objects to this.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 18, 2014

Contributor

BTW, I imagine it would make sense to also add R.ppipe() to complement R.pcompose()?

Contributor

yuri commented Nov 18, 2014

BTW, I imagine it would make sense to also add R.ppipe() to complement R.pcompose()?

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

@fyyyyy , i think @yuri has some good reasons for using the specific logic in isThennable. hasMethod is really intended to dispatch R functions to algebraic types and integration with other libs, e.g. Bacon.js et al.

anyways, I am good with this PR. 👍

Everyone on board?

Member

buzzdecafe commented Nov 18, 2014

@fyyyyy , i think @yuri has some good reasons for using the specific logic in isThennable. hasMethod is really intended to dispatch R functions to algebraic types and integration with other libs, e.g. Bacon.js et al.

anyways, I am good with this PR. 👍

Everyone on board?

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

👍 for ppipe

as to hasMethod, do as you prefer, either is fine by me

Member

buzzdecafe commented Nov 18, 2014

👍 for ppipe

as to hasMethod, do as you prefer, either is fine by me

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 18, 2014

Member

🌿

Member

CrossEye commented Nov 18, 2014

🌿

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 18, 2014

Contributor

Added ppipe().

Contributor

yuri commented Nov 18, 2014

Added ppipe().

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

@yuri thanks for your patience, this has been a long discussion 😣

Member

buzzdecafe commented Nov 18, 2014

@yuri thanks for your patience, this has been a long discussion 😣

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

now the fun part--can you squash to one commit please? 😈

Member

buzzdecafe commented Nov 18, 2014

now the fun part--can you squash to one commit please? 😈

@davidchambers

This comment has been minimized.

Show comment
Hide comment
@davidchambers

davidchambers Nov 18, 2014

Member

I'm not saying these names are better, but let's at least consider other options:

  • R.pCompose and R.pPipe
  • R.composeP and R.pipeP

I actually like the names R.pcompose and R.ppipe, but they're not consistent with R.lPartial and R.rPartial.

Member

davidchambers commented Nov 18, 2014

I'm not saying these names are better, but let's at least consider other options:

  • R.pCompose and R.pPipe
  • R.composeP and R.pipeP

I actually like the names R.pcompose and R.ppipe, but they're not consistent with R.lPartial and R.rPartial.

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 18, 2014

Contributor

@davidchambers Either of those options look good to me. Another option would be R.composeAsync and R.pipeAsync.

Contributor

yuri commented Nov 18, 2014

@davidchambers Either of those options look good to me. Another option would be R.composeAsync and R.pipeAsync.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

i think async is too broad--but i could also get behind composeThen/pipeThen

Member

buzzdecafe commented Nov 18, 2014

i think async is too broad--but i could also get behind composeThen/pipeThen

@kedashoe

This comment has been minimized.

Show comment
Hide comment
@kedashoe

kedashoe Nov 18, 2014

Contributor

R.pCompose and R.pPipe

👍

And I believe according to ramda style isThennable should be _isThennable and makeMultiArgCompose should be _makeMultiArgCompose. Or how about just _makeComposition for the latter?

Contributor

kedashoe commented Nov 18, 2014

R.pCompose and R.pPipe

👍

And I believe according to ramda style isThennable should be _isThennable and makeMultiArgCompose should be _makeMultiArgCompose. Or how about just _makeComposition for the latter?

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 18, 2014

Contributor

@buzzdecafe Maybe R.composeWithThen and R.pipeWithThen()?

@kedashoe Good point on underscores. Not sure about _makeComposition, but I'll go with whatever the maintainers prefer.

Contributor

yuri commented Nov 18, 2014

@buzzdecafe Maybe R.composeWithThen and R.pipeWithThen()?

@kedashoe Good point on underscores. Not sure about _makeComposition, but I'll go with whatever the maintainers prefer.

@davidchambers

This comment has been minimized.

Show comment
Hide comment
@davidchambers

davidchambers Nov 18, 2014

Member

I believe according to ramda style isThennable should be _isThennable

Thank you for helping to enforce this style, @kedashoe. :)

Member

davidchambers commented Nov 18, 2014

I believe according to ramda style isThennable should be _isThennable

Thank you for helping to enforce this style, @kedashoe. :)

@kedashoe

This comment has been minimized.

Show comment
Hide comment
@kedashoe

kedashoe Nov 18, 2014

Contributor

Not sure about _makeComposition, but I'll go with whatever the maintainers prefer.

Actually in regards to this, possibly https://github.com/ramda/ramda/pull/534/files can provide some guidance.. maybe _createComposer in this case? But of course, go with what you like or as you say, whatever the maintainers prefer 😄

Contributor

kedashoe commented Nov 18, 2014

Not sure about _makeComposition, but I'll go with whatever the maintainers prefer.

Actually in regards to this, possibly https://github.com/ramda/ramda/pull/534/files can provide some guidance.. maybe _createComposer in this case? But of course, go with what you like or as you say, whatever the maintainers prefer 😄

@davidchambers

This comment has been minimized.

Show comment
Hide comment
@davidchambers

davidchambers Nov 18, 2014

Member

_createComposer sounds good to me!

Member

davidchambers commented Nov 18, 2014

_createComposer sounds good to me!

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

bach.js

Member

buzzdecafe commented Nov 18, 2014

bach.js

@CrossEye

This comment has been minimized.

Show comment
Hide comment
Member

CrossEye commented Nov 18, 2014

Compose Joke

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

compose(byrd, faure, cage)

Member

buzzdecafe commented Nov 18, 2014

compose(byrd, faure, cage)

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 18, 2014

Contributor

Anyone has any thoughts on R.composeWithThen and R.pipeWithThen as alternative names?

Contributor

yuri commented Nov 18, 2014

Anyone has any thoughts on R.composeWithThen and R.pipeWithThen as alternative names?

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 18, 2014

Member

aw, we gotta get back on topic?! I guess I would go with @davidchambers and @kedashoe pCompose and pPipe for API consistency's sake

Member

buzzdecafe commented Nov 18, 2014

aw, we gotta get back on topic?! I guess I would go with @davidchambers and @kedashoe pCompose and pPipe for API consistency's sake

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 18, 2014

Member

👍 on pCompose and pPipe, even if that last makes me feel as though I'm stuttering.

Member

CrossEye commented Nov 18, 2014

👍 on pCompose and pPipe, even if that last makes me feel as though I'm stuttering.

Adding functions pCompose and pPipe.
R.pCompose works like R.compose, but handles promises.
The resulting function will evaluate to a promise if
any one of the composed functions returns a promise.
If all of the composed functions return something other
than a promise, then the behaviour is identical to that
of R.compose(). In this sense, R.pCompose() is backwards
compatible with R.compose.

R.pPipe is like R.pCompose but takes the arguments in the opposite order.
@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 19, 2014

Contributor

@buzzdecafe Squashed, should be good to go.

Contributor

yuri commented Nov 19, 2014

@buzzdecafe Squashed, should be good to go.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Nov 19, 2014

Member

Nicely done! Thanks for all the hard work, and for sticking with it.

Member

CrossEye commented Nov 19, 2014

Nicely done! Thanks for all the hard work, and for sticking with it.

CrossEye added a commit that referenced this pull request Nov 19, 2014

Merge pull request #524 from yuri/master
Changing _compose() to support promises.

@CrossEye CrossEye merged commit 4516a38 into ramda:master Nov 19, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Nov 19, 2014

Contributor

Thanks everyone, it was great fun!

Contributor

yuri commented Nov 19, 2014

Thanks everyone, it was great fun!

@davidchambers

This comment has been minimized.

Show comment
Hide comment
@davidchambers

davidchambers Nov 19, 2014

Member

Thanks, @yuri!

Member

davidchambers commented Nov 19, 2014

Thanks, @yuri!

@yuri yuri changed the title from Changing _compose() to support promises. to Adding promise-aware function composition. Nov 19, 2014

@davidchambers davidchambers referenced this pull request Nov 19, 2014

Closed

0.8.0 upgrade guide #552

@DrBoolean

This comment has been minimized.

Show comment
Hide comment
@DrBoolean

DrBoolean Nov 20, 2014

Since Promises are functors (with map = then), the standard solution to the example should be:
R.compose(map(R.compose(double, triple)), squareAsync)

But i see that for people new to fp this feature is very accommodating.

Since Promises are functors (with map = then), the standard solution to the example should be:
R.compose(map(R.compose(double, triple)), squareAsync)

But i see that for people new to fp this feature is very accommodating.

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Nov 20, 2014

Member

Yeah, it would be nice if we could be confident that arbitrary promises implementations would behave like functors, but I don't think we can. Also, that would mean having some way of dispatching to then when the promise is passed to map--not a huge deal, but one more branch. And if someone wants to use standard compose for their well-behaved Promise functors, they can do so, as you illustrate above.

Member

buzzdecafe replied Nov 20, 2014

Yeah, it would be nice if we could be confident that arbitrary promises implementations would behave like functors, but I don't think we can. Also, that would mean having some way of dispatching to then when the promise is passed to map--not a huge deal, but one more branch. And if someone wants to use standard compose for their well-behaved Promise functors, they can do so, as you illustrate above.

@dannyko

This comment has been minimized.

Show comment
Hide comment
@dannyko

dannyko Apr 12, 2015

can this be compared to / combined with promises+generators for "piping" async function in ES6? e.g. see http://taskjs.org/ for an analogous code snippet defining an "asynchronous pipe". From the surface, the Ramda.pipeP approach seems cleaner and simpler than the taskjs approach, but I'm not an expert so if anyone has a better understanding of the pros/cons of these two approaches, or whether they can be combined somehow, I'd be very interested to know your thoughts. Thanks

dannyko commented Apr 12, 2015

can this be compared to / combined with promises+generators for "piping" async function in ES6? e.g. see http://taskjs.org/ for an analogous code snippet defining an "asynchronous pipe". From the surface, the Ramda.pipeP approach seems cleaner and simpler than the taskjs approach, but I'm not an expert so if anyone has a better understanding of the pros/cons of these two approaches, or whether they can be combined somehow, I'd be very interested to know your thoughts. Thanks

@yuri

This comment has been minimized.

Show comment
Hide comment
@yuri

yuri Apr 14, 2015

Contributor

@dannyko: I am not quite sure in what way taskjs is similar. It seems to try to fix a rather different problem. But in terms of using generators in an "await"-like manner, R.pipeP should work fine for that. I mean, you should be able to just "yield" the return value of a function that was composed using R.pipeP.

Contributor

yuri commented Apr 14, 2015

@dannyko: I am not quite sure in what way taskjs is similar. It seems to try to fix a rather different problem. But in terms of using generators in an "await"-like manner, R.pipeP should work fine for that. I mean, you should be able to just "yield" the return value of a function that was composed using R.pipeP.

@dannyko

This comment has been minimized.

Show comment
Hide comment
@dannyko

dannyko Apr 14, 2015

thanks! to me, they are both similar in that they both aim to make promises easier to combine, in particular to create a "chain" (or pipe) of evaluations that blocks on promises. Perhaps I will better appreciate the differences once I try using them at some point :)

dannyko commented Apr 14, 2015

thanks! to me, they are both similar in that they both aim to make promises easier to combine, in particular to create a "chain" (or pipe) of evaluations that blocks on promises. Perhaps I will better appreciate the differences once I try using them at some point :)

@joshperry joshperry referenced this pull request in lodash/lodash Dec 14, 2016

Closed

yieldable/thenable compose/pipe #2888

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