From 7e21b3561b0f354c5f5fadd2a9657af12fe1b631 Mon Sep 17 00:00:00 2001 From: Haroen Viaene <hello@haroen.me> Date: Fri, 20 Dec 2024 10:08:57 +0100 Subject: [PATCH] feat(helper): persistHierarchicalRootCount: true (#6478) * feat(helper): persistHierarchicalRootCount: true BREAKING CHANGE: the default behaviour of the helper is now persistHierarchicalRootCount: true * fix(options): remove persistHierarchicalRootCount option It behaves as persistHierarchicalRootCount: true --- packages/algoliasearch-helper/index.d.ts | 1 - .../src/SearchResults/index.js | 42 +---- .../SearchParameters/search.dataset.js | 1 - .../test/spec/SearchResults/getFacetValues.js | 48 +---- .../spec/algoliasearch.helper/searchOnce.js | 153 ++++++++-------- .../spec/hierarchical-facets/simple-usage.js | 171 +----------------- .../test/spec/hierarchical-facets/sort-by.js | 30 ++- .../src/__tests__/instantsearch.test.tsx | 25 +-- .../instantsearch-core/src/instantsearch.ts | 5 +- .../src/types/instantsearch.ts | 10 - .../widgets/__tests__/index-widget.test.ts | 1 - 11 files changed, 107 insertions(+), 380 deletions(-) diff --git a/packages/algoliasearch-helper/index.d.ts b/packages/algoliasearch-helper/index.d.ts index 3dc6603c51..e3ea213ba4 100644 --- a/packages/algoliasearch-helper/index.d.ts +++ b/packages/algoliasearch-helper/index.d.ts @@ -1114,7 +1114,6 @@ declare namespace algoliasearchHelper { * This is for internal use, e.g., avoiding caching in infinite hits, or delaying the display of these results. */ __isArtificial?: boolean | undefined; - persistHierarchicalRootCount?: boolean; } type ISearchResponse<T> = Omit<SearchResponse<T>, 'facets' | 'params'> & diff --git a/packages/algoliasearch-helper/src/SearchResults/index.js b/packages/algoliasearch-helper/src/SearchResults/index.js index 453a063699..e501c490db 100644 --- a/packages/algoliasearch-helper/src/SearchResults/index.js +++ b/packages/algoliasearch-helper/src/SearchResults/index.js @@ -240,11 +240,8 @@ function SearchResults(state, results, options) { }); // Make every key of the result options reachable from the instance - var opts = defaultsPure(options, { - persistHierarchicalRootCount: false, - }); - Object.keys(opts).forEach(function (key) { - self[key] = opts[key]; + Object.keys(options || {}).forEach(function (key) { + self[key] = options[key]; }); /** @@ -508,16 +505,10 @@ function SearchResults(state, results, options) { return; } - self.hierarchicalFacets[position][attributeIndex].data = - self.persistHierarchicalRootCount - ? defaultsPure( - self.hierarchicalFacets[position][attributeIndex].data, - facetResults - ) - : defaultsPure( - facetResults, - self.hierarchicalFacets[position][attributeIndex].data - ); + self.hierarchicalFacets[position][attributeIndex].data = defaultsPure( + self.hierarchicalFacets[position][attributeIndex].data, + facetResults + ); } else { position = disjunctiveFacetsIndices[dfacet]; @@ -589,28 +580,7 @@ function SearchResults(state, results, options) { return; } - // when we always get root levels, if the hits refinement is `beers > IPA` (count: 5), - // then the disjunctive values will be `beers` (count: 100), - // but we do not want to display - // | beers (100) - // > IPA (5) - // We want - // | beers (5) - // > IPA (5) - // @MAJOR: remove this legacy behaviour in next major version - var defaultData = {}; - - if ( - currentRefinement.length > 0 && - !self.persistHierarchicalRootCount - ) { - var root = currentRefinement[0].split(separator)[0]; - defaultData[root] = - self.hierarchicalFacets[position][attributeIndex].data[root]; - } - self.hierarchicalFacets[position][attributeIndex].data = defaultsPure( - defaultData, facetResults, self.hierarchicalFacets[position][attributeIndex].data ); diff --git a/packages/algoliasearch-helper/test/datasets/SearchParameters/search.dataset.js b/packages/algoliasearch-helper/test/datasets/SearchParameters/search.dataset.js index 7ab07b44df..4ff8f7c50b 100644 --- a/packages/algoliasearch-helper/test/datasets/SearchParameters/search.dataset.js +++ b/packages/algoliasearch-helper/test/datasets/SearchParameters/search.dataset.js @@ -336,7 +336,6 @@ function getData() { index: 'test_hotels-node', hitsPerPage: 20, nbHits: 4, - persistHierarchicalRootCount: false, nbPages: 1, page: 0, params: diff --git a/packages/algoliasearch-helper/test/spec/SearchResults/getFacetValues.js b/packages/algoliasearch-helper/test/spec/SearchResults/getFacetValues.js index 53b38e39f0..33b6f80b68 100644 --- a/packages/algoliasearch-helper/test/spec/SearchResults/getFacetValues.js +++ b/packages/algoliasearch-helper/test/spec/SearchResults/getFacetValues.js @@ -751,12 +751,10 @@ test('getFacetValues(facetName) prefers the "main" facet result (disjunctive)', expect(facetValues).toEqual(expected); }); -test('getFacetValues(facetName) prefers the "main" facet result (hierarchical) (persistHierarchicalRootCount: true)', function () { +test('getFacetValues(facetName) prefers the "main" facet result (hierarchical)', function () { var data = require('./getFacetValues/hierarchical-non-exhaustive.json'); var searchParams = new SearchParameters(data.state); - var result = new SearchResults(searchParams, data.content.results, { - persistHierarchicalRootCount: true, - }); + var result = new SearchResults(searchParams, data.content.results); var facetValues = result.getFacetValues('brand').data; @@ -792,45 +790,3 @@ test('getFacetValues(facetName) prefers the "main" facet result (hierarchical) ( expect(facetValues).toEqual(expected); }); - -test('getFacetValues(facetName) prefers the "main" facet result (hierarchical) (persistHierarchicalRootCount: false)', function () { - var data = require('./getFacetValues/hierarchical-non-exhaustive.json'); - var searchParams = new SearchParameters(data.state); - var result = new SearchResults(searchParams, data.content.results, { - persistHierarchicalRootCount: false, - }); - - var facetValues = result.getFacetValues('brand').data; - - var expected = [ - { - count: 50, - data: null, - isRefined: true, - name: 'Apple', - escapedValue: 'Apple', - path: 'Apple', - exhaustive: true, - }, - { - count: 551, - data: null, - isRefined: false, - name: 'Insignia™', - escapedValue: 'Insignia™', - path: 'Insignia™', - exhaustive: true, - }, - { - count: 551, - data: null, - isRefined: false, - name: 'Samsung', - escapedValue: 'Samsung', - path: 'Samsung', - exhaustive: true, - }, - ]; - - expect(facetValues).toEqual(expected); -}); diff --git a/packages/algoliasearch-helper/test/spec/algoliasearch.helper/searchOnce.js b/packages/algoliasearch-helper/test/spec/algoliasearch.helper/searchOnce.js index 0d9e5141ca..bad494c381 100644 --- a/packages/algoliasearch-helper/test/spec/algoliasearch.helper/searchOnce.js +++ b/packages/algoliasearch-helper/test/spec/algoliasearch.helper/searchOnce.js @@ -3,7 +3,7 @@ var algoliasearchHelper = require('../../../index'); var SearchParameters = require('../../../src/SearchParameters'); -test('searchOnce should call the algolia client according to the number of refinements and call callback with no error and with results when no error', function (done) { +test('searchOnce should call the algolia client according to the number of refinements and call callback with no error and with results when no error', async function () { var testData = require('../../datasets/SearchParameters/search.dataset')(); var client = { @@ -21,76 +21,72 @@ test('searchOnce should call the algolia client according to the number of refin .addDisjunctiveFacetRefinement('city', 'Paris') .addDisjunctiveFacetRefinement('city', 'New York'); - helper.searchOnce(parameters, function (err, data) { - expect(err).toBe(null); - - // shame deepclone, to remove any associated methods coming from the results - expect(JSON.parse(JSON.stringify(data))).toEqual( - JSON.parse(JSON.stringify(testData.responseHelper)) - ); - - var cityValues = data.getFacetValues('city'); - var expectedCityValues = [ - { name: 'Paris', escapedValue: 'Paris', count: 3, isRefined: true }, - { name: 'New York', escapedValue: 'New York', count: 1, isRefined: true }, - { - name: 'San Francisco', - escapedValue: 'San Francisco', - count: 1, - isRefined: false, - }, - ]; - - expect(cityValues).toEqual(expectedCityValues); - - var cityValuesCustom = data.getFacetValues('city', { - sortBy: ['count:asc', 'name:asc'], - }); - var expectedCityValuesCustom = [ - { name: 'New York', escapedValue: 'New York', count: 1, isRefined: true }, - { - name: 'San Francisco', - escapedValue: 'San Francisco', - count: 1, - isRefined: false, - }, - { name: 'Paris', escapedValue: 'Paris', count: 3, isRefined: true }, - ]; - - expect(cityValuesCustom).toEqual(expectedCityValuesCustom); - - var cityValuesFn = data.getFacetValues('city', { - sortBy: function (a, b) { - return a.count - b.count; - }, - }); - var expectedCityValuesFn = [ - { name: 'New York', escapedValue: 'New York', count: 1, isRefined: true }, - { - name: 'San Francisco', - escapedValue: 'San Francisco', - count: 1, - isRefined: false, - }, - { name: 'Paris', escapedValue: 'Paris', count: 3, isRefined: true }, - ]; - - expect(cityValuesFn).toEqual(expectedCityValuesFn); - - expect(client.search).toHaveBeenCalledTimes(1); - - var queries = client.search.mock.calls[0][0]; - for (var i = 0; i < queries.length; i++) { - var query = queries[i]; - expect(query.query).toBeUndefined(); - expect(query.params.query).toBeUndefined(); - } - - done(); + const { content } = await helper.searchOnce(parameters); + + // shame deepclone, to remove any associated methods coming from the results + expect(JSON.parse(JSON.stringify(content))).toEqual( + JSON.parse(JSON.stringify(testData.responseHelper)) + ); + + var cityValues = content.getFacetValues('city'); + var expectedCityValues = [ + { name: 'Paris', escapedValue: 'Paris', count: 3, isRefined: true }, + { name: 'New York', escapedValue: 'New York', count: 1, isRefined: true }, + { + name: 'San Francisco', + escapedValue: 'San Francisco', + count: 1, + isRefined: false, + }, + ]; + + expect(cityValues).toEqual(expectedCityValues); + + var cityValuesCustom = content.getFacetValues('city', { + sortBy: ['count:asc', 'name:asc'], }); + var expectedCityValuesCustom = [ + { name: 'New York', escapedValue: 'New York', count: 1, isRefined: true }, + { + name: 'San Francisco', + escapedValue: 'San Francisco', + count: 1, + isRefined: false, + }, + { name: 'Paris', escapedValue: 'Paris', count: 3, isRefined: true }, + ]; + + expect(cityValuesCustom).toEqual(expectedCityValuesCustom); + + var cityValuesFn = content.getFacetValues('city', { + sortBy: function (a, b) { + return a.count - b.count; + }, + }); + var expectedCityValuesFn = [ + { name: 'New York', escapedValue: 'New York', count: 1, isRefined: true }, + { + name: 'San Francisco', + escapedValue: 'San Francisco', + count: 1, + isRefined: false, + }, + { name: 'Paris', escapedValue: 'Paris', count: 3, isRefined: true }, + ]; + + expect(cityValuesFn).toEqual(expectedCityValuesFn); + + expect(client.search).toHaveBeenCalledTimes(1); + + var queries = client.search.mock.calls[0][0]; + for (var i = 0; i < queries.length; i++) { + var query = queries[i]; + expect(query.query).toBeUndefined(); + expect(query.params.query).toBeUndefined(); + } }); -test('searchOnce should call the algolia client according to the number of refinements and call callback with error and no results when error', function (done) { +test('searchOnce should call the algolia client according to the number of refinements and call callback with error and no results when error', async function () { var error = { message: 'error' }; var client = { search: jest.fn().mockImplementationOnce(function () { @@ -107,19 +103,14 @@ test('searchOnce should call the algolia client according to the number of refin .addDisjunctiveFacetRefinement('city', 'Paris') .addDisjunctiveFacetRefinement('city', 'New York'); - helper.searchOnce(parameters, function (err, data) { - expect(err).toBe(error); - expect(data).toBe(null); - - expect(client.search).toHaveBeenCalledTimes(1); + await expect(() => helper.searchOnce(parameters)).rejects.toEqual(error); - var queries = client.search.mock.calls[0][0]; - for (var i = 0; i < queries.length; i++) { - var query = queries[i]; - expect(query.query).toBeUndefined(); - expect(query.params.query).toBeUndefined(); - } + expect(client.search).toHaveBeenCalledTimes(1); - done(); - }); + var queries = client.search.mock.calls[0][0]; + for (var i = 0; i < queries.length; i++) { + var query = queries[i]; + expect(query.query).toBeUndefined(); + expect(query.params.query).toBeUndefined(); + } }); diff --git a/packages/algoliasearch-helper/test/spec/hierarchical-facets/simple-usage.js b/packages/algoliasearch-helper/test/spec/hierarchical-facets/simple-usage.js index 54230e4437..f3b60af493 100644 --- a/packages/algoliasearch-helper/test/spec/hierarchical-facets/simple-usage.js +++ b/packages/algoliasearch-helper/test/spec/hierarchical-facets/simple-usage.js @@ -86,7 +86,7 @@ describe('hierarchical facets: simple usage', function () { client.search.mockClear(); }); - test('persistHierarchicalRootCount: false', function (done) { + test('simple usage', function (done) { var helper = algoliasearchHelper(client, indexName, { hierarchicalFacets: [ { @@ -103,175 +103,6 @@ describe('hierarchical facets: simple usage', function () { helper.toggleFacetRefinement('categories', 'beers > IPA > Flying dog'); - var expectedHelperResponse = [ - { - name: 'categories', - count: null, - isRefined: true, - path: null, - escapedValue: null, - exhaustive: true, - data: [ - { - name: 'beers', - path: 'beers', - escapedValue: 'beers', - count: 9, - isRefined: true, - exhaustive: true, - data: [ - { - name: 'IPA', - path: 'beers > IPA', - escapedValue: 'beers > IPA', - count: 9, - isRefined: true, - exhaustive: true, - data: [ - { - name: 'Flying dog', - path: 'beers > IPA > Flying dog', - escapedValue: 'beers > IPA > Flying dog', - count: 3, - isRefined: true, - exhaustive: true, - data: null, - }, - { - name: 'Brewdog punk IPA', - path: 'beers > IPA > Brewdog punk IPA', - escapedValue: 'beers > IPA > Brewdog punk IPA', - count: 6, - isRefined: false, - exhaustive: true, - data: null, - }, - ], - }, - { - name: 'Pale Ale', - path: 'beers > Pale Ale', - escapedValue: 'beers > Pale Ale', - count: 10, - isRefined: false, - exhaustive: true, - data: null, - }, - { - name: 'Stout', - path: 'beers > Stout', - escapedValue: 'beers > Stout', - count: 1, - isRefined: false, - exhaustive: true, - data: null, - }, - ], - }, - { - name: 'fruits', - path: 'fruits', - escapedValue: 'fruits', - count: 5, - isRefined: false, - exhaustive: true, - data: null, - }, - { - name: 'sales', - path: 'sales', - escapedValue: 'sales', - count: 20, - isRefined: false, - exhaustive: true, - data: null, - }, - ], - }, - ]; - - helper.setQuery('a').search(); - - helper.once('result', function (event) { - var queries = client.search.mock.calls[0][0]; - var hitsQuery = queries[0]; - var parentValuesQuery = queries[1]; - var fullParentsValuesQueries = queries.slice(2); - - expect(queries.length).toBe(4); - - expect(hitsQuery.params.facets).toEqual([ - 'categories.lvl0', - 'categories.lvl1', - 'categories.lvl2', - 'categories.lvl3', - ]); - expect(hitsQuery.params.facetFilters).toEqual([ - ['categories.lvl2:beers > IPA > Flying dog'], - ]); - - expect(parentValuesQuery.params.facets).toEqual([ - 'categories.lvl0', - 'categories.lvl1', - 'categories.lvl2', - ]); - expect(parentValuesQuery.params.facetFilters).toEqual([ - ['categories.lvl1:beers > IPA'], - ]); - - // Root - expect(fullParentsValuesQueries[0].params.facets).toEqual( - 'categories.lvl0' - ); - expect(fullParentsValuesQueries[0].params.facetFilters).toBe(undefined); - - // Level 1 - expect(fullParentsValuesQueries[1].params.facets).toEqual( - 'categories.lvl1' - ); - expect(fullParentsValuesQueries[1].params.facetFilters).toEqual([ - 'categories.lvl0:beers', - ]); - - expect(event.results.hierarchicalFacets).toEqual(expectedHelperResponse); - expect( - event.results.hierarchicalFacets.find((f) => f.name === 'categories') - ).toEqual(expectedHelperResponse[0]); - - // we do not yet support multiple values for hierarchicalFacetsRefinements - // but at some point we may want to open multiple leafs of a hierarchical menu - // So we set this as an array so that we do not have to bump major to handle it - expect( - Array.isArray(helper.state.hierarchicalFacetsRefinements.categories) - ).toBeTruthy(); - done(); - }); - }); - - test('persistHierarchicalRootCount: true', function (done) { - var helper = algoliasearchHelper( - client, - indexName, - { - hierarchicalFacets: [ - { - name: 'categories', - attributes: [ - 'categories.lvl0', - 'categories.lvl1', - 'categories.lvl2', - 'categories.lvl3', - ], - }, - ], - }, - { - persistHierarchicalRootCount: true, - } - ); - - helper.toggleFacetRefinement('categories', 'beers > IPA > Flying dog'); - var expectedHelperResponse = [ { name: 'categories', diff --git a/packages/algoliasearch-helper/test/spec/hierarchical-facets/sort-by.js b/packages/algoliasearch-helper/test/spec/hierarchical-facets/sort-by.js index 079d149f4e..3f87b23bca 100644 --- a/packages/algoliasearch-helper/test/spec/hierarchical-facets/sort-by.js +++ b/packages/algoliasearch-helper/test/spec/hierarchical-facets/sort-by.js @@ -1,6 +1,6 @@ 'use strict'; -test('hierarchical facets: using sortBy', function (done) { +test('hierarchical facets: using sortBy', async function () { var algoliasearch = require('algoliasearch'); algoliasearch = algoliasearch.algoliasearch || algoliasearch; @@ -30,6 +30,7 @@ test('hierarchical facets: using sortBy', function (done) { var algoliaResponse = { results: [ + // for hits { query: 'a', index: indexName, @@ -45,6 +46,7 @@ test('hierarchical facets: using sortBy', function (done) { 'categories.lvl2': { 'beers > IPA > Flying dog': 1 }, }, }, + // deepest level { query: 'a', index: indexName, @@ -54,8 +56,8 @@ test('hierarchical facets: using sortBy', function (done) { nbPages: 1, hitsPerPage: 1, facets: { - 'categories.lvl0': { beers: 5 }, - 'categories.lvl1': { 'beers > IPA': 5 }, + 'categories.lvl0': { beers: 1 }, + 'categories.lvl1': { 'beers > IPA': 1 }, 'categories.lvl2': { 'beers > IPA > Flying dog': 1, 'beers > IPA > Anchor steam': 1, @@ -63,6 +65,7 @@ test('hierarchical facets: using sortBy', function (done) { }, }, }, + // root level { query: 'a', index: indexName, @@ -75,6 +78,19 @@ test('hierarchical facets: using sortBy', function (done) { 'categories.lvl0': { beers: 5 }, }, }, + // other levels + { + query: 'a', + index: indexName, + hits: [{ objectID: 'one' }], + nbHits: 1, + page: 0, + nbPages: 1, + hitsPerPage: 1, + facets: { + 'categories.lvl1': { 'beers > IPA': 5 }, + }, + }, ], }; @@ -143,8 +159,10 @@ test('hierarchical facets: using sortBy', function (done) { }); helper.setQuery('a').search(); - helper.once('result', function (event) { - expect(event.results.hierarchicalFacets).toEqual(expectedHelperResponse); - done(); + + const { results } = await new Promise(function (resolve) { + helper.once('result', resolve); }); + + expect(results.hierarchicalFacets).toEqual(expectedHelperResponse); }); diff --git a/packages/instantsearch-core/src/__tests__/instantsearch.test.tsx b/packages/instantsearch-core/src/__tests__/instantsearch.test.tsx index d2e2bbecff..89786f6b9a 100644 --- a/packages/instantsearch-core/src/__tests__/instantsearch.test.tsx +++ b/packages/instantsearch-core/src/__tests__/instantsearch.test.tsx @@ -864,30 +864,7 @@ describe('start', () => { expect(algoliasearchHelper).toHaveBeenCalledWith( searchClient, indexName, - undefined, - { persistHierarchicalRootCount: false } - ); - }); - - it('creates a Helper with `persistHierarchicalRootCount` set to true when specified with a future flag', () => { - const searchClient = createSearchClient(); - const indexName = 'indexName'; - const future = { - persistHierarchicalRootCount: true, - }; - const search = new InstantSearch({ - indexName, - searchClient, - future, - }); - - search.start(); - - expect(algoliasearchHelper).toHaveBeenCalledWith( - searchClient, - indexName, - undefined, - future + undefined ); }); diff --git a/packages/instantsearch-core/src/instantsearch.ts b/packages/instantsearch-core/src/instantsearch.ts index bf08fdc793..05074efd4e 100644 --- a/packages/instantsearch-core/src/instantsearch.ts +++ b/packages/instantsearch-core/src/instantsearch.ts @@ -52,7 +52,6 @@ export const INSTANTSEARCH_FUTURE_DEFAULTS: Required< InstantSearchOptions['future'] > = { preserveSharedStateOnUnmount: false, - persistHierarchicalRootCount: false, }; /** @@ -343,9 +342,7 @@ See documentation: ${createDocumentationLink({ // we need to respect this helper as a way to keep all listeners correct. const mainHelper = this.mainHelper || - algoliasearchHelper(this.client, this.indexName, undefined, { - persistHierarchicalRootCount: this.future.persistHierarchicalRootCount, - }); + algoliasearchHelper(this.client, this.indexName, undefined); mainHelper.search = () => { this.status = 'loading'; diff --git a/packages/instantsearch-core/src/types/instantsearch.ts b/packages/instantsearch-core/src/types/instantsearch.ts index bbca3bfbbe..c03d426a94 100644 --- a/packages/instantsearch-core/src/types/instantsearch.ts +++ b/packages/instantsearch-core/src/types/instantsearch.ts @@ -101,16 +101,6 @@ export type InstantSearchOptions< * @default false */ preserveSharedStateOnUnmount?: boolean; // @MAJOR remove option, only keep the "true" behaviour - /** - * Changes the way root levels of hierarchical facets have their count displayed. - * - * If `false` (by default), the count of the refined root level is updated to match the count of the actively refined parent level. - * - * If `true`, the count of the root level stays the same as the count of all children levels. - * - * @default false - */ - persistHierarchicalRootCount?: boolean; // @MAJOR change the default to true }; }; diff --git a/packages/instantsearch-core/src/widgets/__tests__/index-widget.test.ts b/packages/instantsearch-core/src/widgets/__tests__/index-widget.test.ts index 48ea187006..4884a84f38 100644 --- a/packages/instantsearch-core/src/widgets/__tests__/index-widget.test.ts +++ b/packages/instantsearch-core/src/widgets/__tests__/index-widget.test.ts @@ -3346,7 +3346,6 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/index-widge nbPages: 1, page: 0, params: '', - persistHierarchicalRootCount: false, processingTimeMS: 0, query: 'iphone', };