Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

better compose #643

Merged
merged 3 commits into from
May 15, 2018
Merged

better compose #643

merged 3 commits into from
May 15, 2018

Conversation

zeakd
Copy link
Contributor

@zeakd zeakd commented Apr 9, 2018

Hi. it's not a issue, but made simple compose without 'if'. thanks.

@codecov-io
Copy link

codecov-io commented Apr 9, 2018

Codecov Report

Merging #643 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #643     +/-   ##
=========================================
- Coverage   88.28%   88.18%   -0.1%     
=========================================
  Files          49       49             
  Lines         367      364      -3     
=========================================
- Hits          324      321      -3     
  Misses         43       43
Impacted Files Coverage Δ
src/packages/recompose/compose.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3c743c...6c21607. Read the comment docs.

@istarkov
Copy link
Contributor

istarkov commented Apr 9, 2018

I like such changes, prev realusation of compose usually had same behaviour as here https://github.com/reactjs/redux/blob/master/src/compose.js
I think because of perf reasons for some strange use cases (no need in compose for 1 function or 0 at all) so I think we can accept this cc @wuct your thoughts?

@ryota-murakami
Copy link

interesting change, and i wonder how much performance increase on today's CPU, that has heighly optimized if else branch prediction.
but i'm not familiar V8 engine, so you make me want to know V8 more deeply!

about @istarkov mentioned some strange use cases, i came across through contribute to spectrum).
you could see like export const Card = compose()(CardPure); fix temporarily code:smile:
withspectrum/spectrum@a245109

(cc @mxstbr

@wuct
Copy link
Contributor

wuct commented May 15, 2018

Yes, I think we can accept it. The new approach is the reason why the "identity" function has the name "identity," and I like it. :)

@wuct wuct merged commit 7a9e2d2 into acdlite:master May 15, 2018
@dokai
Copy link

dokai commented Jun 4, 2018

Hi! I came upon this PR while reviewing the proposed upgrades to our app's dependencies.

I appreciate the slightly shorter version of the implementation but had one question come up. It seems the initial motivation was to optimize away the if check and therefore do less work.

However, it seems that this trades a one-time cost at composition time (*) to a run-time cost on each invocation of the composed function due to adding the identity function in the composition chain unconditionally.

I'm not familiar with the details on how the engines would actually treat or optimize this but in general the cost of a function call is usually higher than an if check.

(*) I'm assuming that the common pattern is to bind the composed function to a variable that is used to invoke the function therefore having to evaluate the if only once, e.g.

const myFunc = compose(fn1, fn2, f3);
myFunc('foobar')

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants