Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

fix(disjunctiveFacets): avoid escaping non-string values #902

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Apr 5, 2022

This wasn't fully functioning behaviour before #889 already, as numeric values don't show as refined, even if they are (see

var refinementValueAsString = '' + refinementValue;
where it's purposely casted to a string, but the input values aren't), but it didn't throw an error, which became the case in #889

A customer discovered this in https://algolia.atlassian.net/browse/CR-883

@Haroenv Haroenv requested review from a team, dhayab and FabienMotte and removed request for a team April 5, 2022 13:12
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 5, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7f1c7ca:

Sandbox Source
javascript-helper-app Configuration
InstantSearch.js Configuration

Comment on lines +262 to +263
// even though this could be considered refined, it's not because it's a number (existing bug)
{name: '50', escapedValue: '50', count: 0, isRefined: false}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't changed the behaviour of "injected numeric facets"' isRefined, it really should be true, but that was the case before #889 already

other relevant part of the code:

if (state.disjunctiveFacetsRefinements[dfacet]) {
state.disjunctiveFacetsRefinements[dfacet].forEach(function(refinementValue) {
// add the disjunctive refinements if it is no more retrieved
if (!self.disjunctiveFacets[position].data[refinementValue] &&
state.disjunctiveFacetsRefinements[dfacet].indexOf(unescapeFacetValue(refinementValue)) > -1) {
self.disjunctiveFacets[position].data[refinementValue] = 0;
}
});
}
}

Copy link
Member

@dhayab dhayab left a comment

Choose a reason for hiding this comment

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

Should we update the unescape util in algolia/react-instantsearch#3420 as well, or are the values cast into strings at that time?

@Haroenv
Copy link
Contributor Author

Haroenv commented Apr 5, 2022

3420 is about labels, thus not relevant here :)

@Haroenv Haroenv merged commit 4ac67bd into develop Apr 5, 2022
@Haroenv Haroenv deleted the fix/numeric-disjunctive branch April 5, 2022 13:34
Haroenv added a commit that referenced this pull request Apr 5, 2022
 * fix(disjunctiveFacets): avoid escaping non-string values (#902) 4ac67bd
dhayab pushed a commit to algolia/instantsearch that referenced this pull request Jul 10, 2023
 * fix(disjunctiveFacets): avoid escaping non-string values (algolia/algoliasearch-helper-js#902) algolia/algoliasearch-helper-js@3f7473f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants