itemsapi / itemsjs

Extremely fast faceted search engine in JavaScript - lightweight, flexible, and simple to use
Apache License 2.0
346 stars 41 forks source link

fix error when filter value not found in items set #130

Closed aurelienroux closed 1 year ago

aurelienroux commented 1 year ago

It’s my first time contributing to a public repo so let me know if anything needs to be corrected. I added a test to explain the specific usecase covered.

This pr is adding the possibility of having a non existing value in the filters. We want to have a predefined set of filters, but also having the possibility that the filtered data (items) will sometimes not include one of the options.

example: possible filters for categories would be comedy, drama or thriller by default, but the related items would only contains comedy or drama categories. Added a fallback to an empty FastBitset when needed, so it would not break.

Let me know if any changes are necessary and thanks for the lib!

cigolpl commented 1 year ago

Thank you for your first contribution!

In my opinion we should be returning zero results instead of ignoring not existing filters like in your case "thriller".

The reason is - it directly informs the user that their criteria didn't match anything, prompting them to reconsider their filter choices and users won't be under the impression that they successfully applied all filters when in reality some were invalid.

Would it work with your use case ?

aurelienroux commented 1 year ago

Actually, this is correct when conjoncture:true. But when false, ItemsJS applies the OR operator between filter values, so a non-existing value should just be ignored. I added both tests with different conjunctions to ensure we apply the proper behaviour.

WDYT ?

cigolpl commented 1 year ago

Looks great to me! Merging now.

cigolpl commented 1 year ago

Released new version https://github.com/itemsapi/itemsjs/releases/tag/v2.1.22

aurelienroux commented 1 year ago

awesome, thanks for your help 🙏