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

Add compose function #30

Merged
merged 4 commits into from
Dec 2, 2014
Merged

Add compose function #30

merged 4 commits into from
Dec 2, 2014

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Nov 26, 2014

It's missing the docblock & tests but I wanted to get feedback before I submitted the left-to-right version

Missing docblock & tests.
@tjmehta
Copy link
Owner

tjmehta commented Nov 26, 2014

Hey your logic is definitely sound, but I think I didn't provide you enough info in my last comment.
Basically, what I meant is that compose should be like and and or in that it takes only two arguments.
This way it is more of a foundational method, that users could combine themselves (outside of 101) with reduce or reduceRight to compose as many functions as they'd like in any order.

Also, out of curiosity - have you used compose in any of your applications?
The reason I am asking is that the way 101 came to be is: I added methods that I actually needed in applications that I was coding. I guess based on the nature of my applications I never actually needed a compose method. Compose is definitely a great textbook method, wondering how many people have utilized a compose method in their real world applications.

@jfsiii
Copy link
Contributor Author

jfsiii commented Nov 26, 2014

Sorry, you did say that in the comments. I submitted a version which matched my usage patterns. I'm using compose/pipe now in a web app to create pipelines for things like converting browser events to generic data structures, reading/writing data to/from the DOM, and so on. All those examples use > 2 functions.

To your point about solving needs from your apps (and compose being a textbook method), I find the "only 2 arguments" signature ... less attractive. I believe limiting compose to a 2 value signature, and requiring the user to build their own version which supports n arguments, would hurt usage. Can there also be an n-value version? I believe that's the most common case and don't believe the user should have to implement that on their own.

@tjmehta
Copy link
Owner

tjmehta commented Nov 28, 2014

if 101/compose accepts two args composeAll becomes a one-liner. It doesn't actually require that users implement there own:

var compose = require('101/compose');

var composed = [f, g, h, i, j, k, l].reduceRight(compose);
// composed(x) === f(g(h(i(j(k(l(x))))))) // true
// assuming: compose(f, g)(x) === f(g(x)) // true

What do you think?

@jfsiii
Copy link
Contributor Author

jfsiii commented Nov 28, 2014

I think it's great. This is what you described from the beginning and it works well. I'll submit the tests in a separate commit.

In my projects, I'll likely add something like:

function pipeline(fns) {
  return fns.reduceRight(compose);  
}

because I don't think it's apparent that [f, g, h, i, j, k, l].reduceRight(compose) processes left-to-right, especially with a reduceRight in there. I think pipeline([f, g, h, i, j, k, l]) is much more approachable but I understand if you don't think it fits the project goal or approach.

Thanks for the example and persistence. I'm glad to know the binary compose.

@tjmehta
Copy link
Owner

tjmehta commented Nov 28, 2014

Cool, I'm glad you agree. I can definitely see how it isn't immediately apparent, especially upon seeing this type of behavior for the first time. Because of this we should definitely put that in the Readme as an example, I think that will be enough to clear up the confusion.

If more people ask for this behavior, I will definitely consider adding it. I just want to keep things barebones for as long as I can, or this library might start getting bloated like others.

@tjmehta
Copy link
Owner

tjmehta commented Nov 28, 2014

Thanks for the help! 👍

@tjmehta
Copy link
Owner

tjmehta commented Nov 29, 2014

Btw I think I made a mistake it is just reduce

var compose = require('101/compose');

var composed = [f, g, h, i, j, k, l].reduce(compose);
// composed(x) === f(g(h(i(j(k(l(x))))))) // true
// assuming: compose(f, g)(x) === f(g(x)) // true

Also, would love some tests for this. I am trying to maintain 100% test coverage for 101.
To test just run npm test.

jfsiii and others added 2 commits November 29, 2014 08:46
@tjmehta
Copy link
Owner

tjmehta commented Dec 2, 2014

Fantastic! Thank you!

tjmehta added a commit that referenced this pull request Dec 2, 2014
@tjmehta tjmehta merged commit b988fd7 into tjmehta:master Dec 2, 2014
@tjmehta tjmehta mentioned this pull request Dec 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants