Skip to content

Conversation

@istarkov
Copy link
Contributor

@istarkov istarkov commented Dec 24, 2016

  1. With filtering compose does not have the well know definition of this function as like as in your comments https://github.com/reactjs/redux/blob/master/src/compose.js#L7-L9
  2. Then I accidentally pass something what is not a function into compose, the only thing I could expect is an error not silent code execution. (there is no need at all to pass anything which is not a function)
  3. Typed definition of compose (even you does not use it) will be really hard to create with filtering.

So I suggest you to remove this line.

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Dec 24, 2016

As a developer I expect my code to throw an Error, not eat it. Also I strongly opposite to extra useless computations in redux. Why not to filter functions outside of compose if you really need this?
So big 👍 to this PR

@alexeyraspopov
Copy link
Contributor

This breaks the @evilj0e's use case described in #2073

@alexeyraspopov
Copy link
Contributor

But throwing errors explicitly is definitely a better thing.

@jimbolla
Copy link
Contributor

What if it were to only filter out falsey?

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Dec 24, 2016

@jimbolla why filter something at all?
The problem described in #2073 can be solved in many ways by one-liner. But the fix from #2073 is a fix of a particular codebase problems and it leads to ommited errors and performance regressions in others people code

@chicoxyzzy
Copy link
Contributor

One can filter everything he wants to outside of compose. It's so easy I can't even

@timdorr
Copy link
Member

timdorr commented Dec 30, 2016

Good points being made. Thanks!

@timdorr timdorr merged commit b4fb081 into reduxjs:master Dec 30, 2016
@gaearon gaearon changed the title Remove filtering inside compose Remove filtering inside compose Dec 30, 2016
seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
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.

5 participants