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

Fixes issue with filtering with property values of number 0 #47

Merged
merged 4 commits into from
May 7, 2018

Conversation

pushpinder107
Copy link
Contributor

What:
#46
The filter does not return data if the value of the key being filtered is the number 0

Why:
It is possible that the data may contain properties with values which are numbers (e.g. age) and these properties could have the value 0 as well. Hence, filtering on 0 should be possible.

How:
By adding an additional check to make sure the number 0 is not incorrectly rejected by the if statement.

Checklist:

  • Documentation "N/A"
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks! Just one simple change.

src/index.js Outdated
@@ -387,7 +387,7 @@ function getItemValues(item, key) {
value = item[key]
}
// concat because `value` can be a string or an array
return value ? [].concat(value) : null
return value || value === 0 ? [].concat(value) : null
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather change this to:

return value == null ? [].concat(value) : null

That way we cover null and undefined and all other cases will be concatenate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Will change it.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Hmmm... Somehow this is breaking the build, it looks like it's the test:ts script (runs as part of the validate script). Does the validate script work for you locally?

@pushpinder107
Copy link
Contributor Author

Nope, validate script fails locally :
Failed at the [email protected] validate script.

@kentcdodds
Copy link
Owner

Hmmm... Looks like your changes are not related to this. It's breaking for me locally without your changes. I'll look into it.

@kentcdodds
Copy link
Owner

I'm not sure what happened, but things were very out of date. I've updated everything and it should work now.

@kentcdodds kentcdodds merged commit 3b98826 into kentcdodds:master May 7, 2018
@kentcdodds
Copy link
Owner

Thanks a bunch!

@kentcdodds
Copy link
Owner

🎉 This PR is included in version 2.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@pushpinder107
Copy link
Contributor Author

😁

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.

2 participants