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

composition can be seeded with multiple arguments #1050

Merged
merged 4 commits into from
Dec 12, 2015

Conversation

Bjvanminnen
Copy link
Contributor

@Bjvanminnen Bjvanminnen commented Nov 18, 2015

Related to #632 (comment)

This allows us to provide our composition with multiple arguments.

It does so by pulling off the last function from our list, calling it with the provided args, and using the result as the initialValue for reduceRight (which we then call on the remaining functions). Unfortunately, this does make our simple one liner a little more complex

Let me know if you see a cleaner approach.

@dbrans
Copy link
Contributor

dbrans commented Nov 18, 2015

Hey @Bjvanminnen, If you need a multiple-arg compose in your app could you not use lodash's compose?

@Bjvanminnen
Copy link
Contributor Author

I could, and am fine with doing so. However, it also seems reasonable to expect the compose provided by redux to support this functionality.

@dbrans
Copy link
Contributor

dbrans commented Nov 18, 2015

Watch out for an empty function list. The way compose is written in this PR, compose()(x) will throw an error.

Currently redux's compose()(x) returns x, lodash's, _.compose()(x, y) returns x.

@Bjvanminnen
Copy link
Contributor Author

I would argue that neither redux nor lodash's current behavior really makes sense. I can't think of any good reason one would call compose with no functions, but if you did, I would think it would make more sense for the result to be () => undefined instead of x => x or (x, y) => x.

That said, I understand the value in not changing existing behavior where there's not a clearly better behavior. I will update my PR and add a test for this, hopefully later this evening.

@Bjvanminnen
Copy link
Contributor Author

Latest commit ensures that we don't change existing behavior for case where no funcs are passed.

@@ -6,5 +6,9 @@
* left. For example, compose(f, g, h) is identical to arg => f(g(h(arg))).
*/
export default function compose(...funcs) {
return arg => funcs.reduceRight((composed, f) => f(composed), arg)
return (...args) => {
return funcs.length ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little hard to grasp what this is doing at first.
Maybe re-writing this as an if statement would make this function a bit easier to follow.

@Bjvanminnen
Copy link
Contributor Author

Stopped trying to do this as a one-liner, and I think it makes it a lot easier to follow.

@gaearon
Copy link
Contributor

gaearon commented Nov 27, 2015

Would this be a breaking change?

@Bjvanminnen
Copy link
Contributor Author

It shouldn't be.

If given no funcs, we return the first argument, same as before.
If given funcs and a single argument, behavior should be identical to before.
Only new behavior is that if given multiple arguments, we pass all of them to the last func instead of ignoring the additional arguments.

gaearon added a commit that referenced this pull request Dec 12, 2015
composition can be seeded with multiple arguments
@gaearon gaearon merged commit 9cd5af4 into reduxjs:master Dec 12, 2015
@gaearon
Copy link
Contributor

gaearon commented Jan 25, 2016

Out in 3.0.6, thanks!

@markerikson markerikson mentioned this pull request Dec 10, 2016
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

4 participants