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

Remove filtering inside compose #2167

Merged
merged 1 commit into from
Dec 30, 2016
Merged

Remove filtering inside compose #2167

merged 1 commit into from
Dec 30, 2016

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.

None yet

5 participants