-
Notifications
You must be signed in to change notification settings - Fork 386
fix(sffv): clamp maxFacetHits to the allowed range #1696
Conversation
Deploy preview for react-instantsearch ready! Built with commit ed44141 https://deploy-preview-1696--react-instantsearch.netlify.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Max bundle size will need to be made bigger
function onSearchForFacetValues({ facetName, query, maxFacetHits = 10 }) { | ||
// The values 1, 100 are the min / max values that the engine accepts. | ||
// see: https://www.algolia.com/doc/api-reference/api-parameters/maxFacetHits | ||
const maxFacetHitsWithinRange = Math.max(1, Math.min(maxFacetHits, 100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have this issue as well in InstantSearch.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have kinda the same issue, but it's less visible because we don't use the showMore
option to conditionally pick the value for maxFacetHits
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with build size change
7043274
to
ed44141
Compare
Summary
With the current implementation the value used for
maxFacetHits
is the default limit of the widget or the user provided value. It's fine for most cases, but it happens that the value provided by the user is higher than 100. This leads to an issue because the Algolia engine only accepts values between 1 - 100. When the value is not in that range the API throws an error.I chose to fix the issue inside InstantSearch rather than in the helper because it feels odd (to me) to change the value on behalf of the user. The helper can be directly manipulate by users so it makes sense to throw an error if the value is invalid. With InstantSearch it's a bit different we use the helper, they don't have any knowledge of it. Since we allow any value for the limit it makes sense to send the correct a value to the API.
The fix clamp the provided value in the range 1, 100. I've also create a default value of 10 for the
maxFacetHits
in case no value is provided. It avoid to gets aNaN
value fromMath.{min,max}
. It doesn't breaks anything because10
is the default value on the engine.Before
After