Simplify composer

[JoeCortopassi]

cleaned up the composer so that it doesn't rely on slicing and reversing the array of functions

[toddw]

Nice! That definitely makes the code a lot cleaner and easier to understand

[markerikson]

My FP-fu is admittedly kinda weak. First issue I have here is I don't know if the changes actually keep the order of operations correct. Second issue is that even if the existing code isn't the absolute prettiest, it's there and it works. I'd need a pretty compelling reason to make changes to it at this point.

Now, I do see a compose.spec.js test file, and it looks like all existing tests do pass. That's definitely a starting point. I'm just not sure that there's enough of a reason to make changes.

[gaearon]

Might be a good idea to look through the revision history to see how it got to be this way.
I don't remember but we changed it a few times.

[markerikson]

Yeah. As I've said a couple times, given Redux's current status and my own relative newness as a maintainer, my default attitude towards code change requests is "No unless it's really obvious it's a good thing".

[JoeCortopassi]

I think the main argument for the change is this: if something broke concerning the current implementation would you be able to a) understand what it’s doing, b ) explain it to someone else, c) be sure it wasn’t the source of the error. The update makes it easier to parse, and actually makes the composition function more easily testable.

@gaearon history doesn't really show a great reason for it to exist that way. First commit has it using reduceRight with the head/rest chopping as well. Seems like the main changes that were made over time were things like guarding against non-functions being passed or short-circuits for stuff like single item arrays

[markerikson]

Did some digging through the history. Looks like the current iteration was made in #1050 .

[JoeCortopassi]

@markerikson that actually makes me feel like this change is even more important. The current implementation isn't a true compose. Given >1 arguments, the first function will be evaluated, before being composed with the rest. That seems broken

[JoeCortopassi]

I mean, the whole thing can be boiled down to

funcs.filter(f=>typeof f === "function").reduce((a, b) => (...args) => a(b(...args)), i=>i)

but I wanted to be as low impact as possible

[markerikson]

As I said, my own FP-fu is fairly weak. I'm going to ask you to simplify and clarify the issues involved :)

• What is a "true compose", and how does the current implementation differ? What about that difference is "broken"? What impact does it have?
• What about the current implementation is "evaluating the first function"?

[JoeCortopassi]

@markerikson the last part last(...args) in reduceRight is evalutating, not composing. That is broken

[JoeCortopassi]

A true compose is binding many functions together (think chaining) such that it reproduces one function that is callable at a later time.

semi-bad example that should help demonstrate:

function add10 (num) { return num+10; }
function multiply10 (num) { return num*10; }
function subtract10 (num) { return num-10; }

const compose = (...funcs) => funcs.reduce((a, b) => (...args) => a(b(...args)), i=>i);

const addMultiplySubtract = compose(add10, multiply10, subtract10);
const subtractMultiplyAdd = compose(subtract10, multiply10, add10);

addMultiplySubtract(5) === subtractMultiplyAdd(5) // false. Order of operations matter

this is why getting order of operations right important for redux/middleware/etc

[markerikson]

Sorta following the concern here. Does the fact that this is normally used with store enhancers affect the usage/behavior in any way?

[gaearon]

Please add tests that fail with master.

[JoeCortopassi]

@gaearon there isn't really a test against it. It's just not really composing. It's using closures to make it ok to evaluate the last function as reduceRight's initial value.

I didn't really expect this to be a big deal, I just noticed it while playing with some stuff and was just trying to help out. Thought it would be pretty obvious that it had some rust from changes over time by stuff like const last = funcs[funcs.length - 1] being used rather than const last = funcs.pop(). It could even be changed to

export default function compose(...funcs) {
  if (funcs.length === 0) {
    return arg => arg
  }

  funcs = funcs.filter(func => typeof func === 'function')

  if (funcs.length === 1) {
    return funcs[0]
  }

  const last = funcs.pop()
  return (...args) => funcs.reduceRight((composed, f) => f(composed), last(...args))
}

I just figured that if I was fixing that, I might as well update the reduceRight as well. If we wanted to get more rigorous and have something that added a test, I guess I could add a check in case a method was trying to be composed as well, in which case we'd get a type error when it was eventually called whether it was bound or not

[gaearon]

I’m having a hard time following this because I’m not really a smart programer.
So I am asking you to help me by providing unit tests that demonstrate the observable difference in behavior.

If it’s calling functions too early, we could test this by setting a flag inside and checking if the flag gets set during compose call or during a call on the result function. Would that make sense?

Thank you for your effort! I appreciate your help but my concerns are coming from a good intent, not from being stubborn or self-righteous about a broken implementation (see also #2146 (comment)).

It’s just that if this is an observable change and people might rely on it, it should go into the next major.

[JoeCortopassi]

@gaearon #2033 looks like it's caused by a transpiler being confused by the current implementation as well. I assume a non-trivial portion of Redux users are using babel or something, which would mean that you've likely seen errors caused by this, but not clearly traceable back to this.

You're clearly a smart and accomplished programmer, this isn't about ability. Ask my coworkers, I'm nothing special. We just use and love redux and I wanted to help out. I saw something I thought I could make better, and I'm sure someone will eventually see glaring problems with my patch and make it better as well. That's the benefit/nature of open source. It's an area that already has good tests, and it's a low impact change, if it causes side effects that are completely unknown (it shouldn't), then now we know a new case to test for and it's easy to revert. Admittedly, I'm a big fan of undocumented features/implementation being open game to be changed out from under people (i.e. don't play out of bounds), but I don't believe that's what's being done here

[JoeCortopassi]

Interesting, this might be the conversation about why this happened that you were thinking of #632 (comment)

Looks like the reduceRight and last/rest were more about being able to hack in multiple args being accepted. This PR should make it less hacky as a side effect

[markerikson]

Isn't code history spelunking fun? :)

[jridgewell]

This PR and the original code are equivalent, minus the performance.

• The old code creates 2 closures (1 could be hoisted, avoiding the allocation every time) and calls apply 1 time.
• The new code creates n - 2 closures, and calls apply n times.

There's no observable eager evaluation happening, unless you monkeypatch Array#reduceRight, and that's unlikely.

[JoeCortopassi]

@jridgewell timed over 10,000 iterations for each situation. It's doubly faster in the most common situation, and ranges between 3x faster and simply matching in other situations

it's intentional that they are functionally equivalent. The new version should be more readable/predictable, doesn't do weird stuff like const last = funcs[funcs.length - 1] instead of just const last = funcs.pop(), and as a side effect, it's faster. If speed was the goal though, I'd probably move the composer lambda used by reduce, and the lambda used by filter outside of the actual call so that they don't have an environment record created every use

[markerikson]

Interesting. Granted, the typical use case is "call once while setting up the store", but I suppose it's possible that someone out there might be reusing it for something more complicated. Thanks for the benchmarks.

[JoeCortopassi]

@markerikson 10000 iterations was just to prevent random issues with the computer from skewing the average. By common use case, I simply meant it being passed in between 1-10 items for composition.

[jridgewell]

It's doubly faster in the most common situation, and ranges between 3x faster and simply matching in other situations

It depends on what you want to prioritize for, the current code is by no means optimized. #reduceRight and all the array iteration methods are terribly slow compared for simple for loops, creating functions slows down startup time, etc. It may be that readability is more important than speed. But even then, I'd argue the current code is easier to understand than the composing closure functions (though yours is extremely beautiful, I love reading these solutions I'd never come up with).

http://justin.ridgewell.name/browser-benchmark/redux-compose/

[gaearon]

I’m good with making this change.
#yolo

[jimbolla]

I like this change. But also, shouldn't the filter: funcs = funcs.filter(func => typeof func === 'function') be before the length===0 check? If someone calls

compose(
  isDev ? someEnhancer : null,
  isDev ? anotherEnhancer : null
)

it would miss out on that check.

Also, recompose has the same function. I'd suggest also submitting this code change there. Ideally the implementations would be exactly the same to aid code minification.

[JoeCortopassi]

@jimbolla @gaearon I can add the filter move to this pr or put it in a separate one. Personally I'd rather merge this pr and put that change in a separate pr, but I'm really good with whatever, just let me know :-)

[JoeCortopassi]

Filter change pr
https://github.com/reactjs/redux/pull/2147/files

[JoeCortopassi]

so is this good to go?

[markerikson]

Sorry, been focused on other tasks the last couple days. I see a 👍 from Dan, so let's do this.

[markerikson]

Thanks for the assistance, @gaearon and @jimbolla , and thank you @JoeCortopassi for the fixes!