Skip to content

Commit

Permalink
chore(enhanceConfiguration): swap mergeDeep for mergeSearchParameters (
Browse files Browse the repository at this point in the history 
#3945)

* chore(enhanceConfiguration): swap mergeDeep for mergeSearchParameters

This is a follow-up on #3917, to easily be able to see the perf impact this has. It's expected that we lose some performance, but it's a tradeoff we can make to fix some bugs in wrongly merging parameters

* chore: remove mergeDeep
  • Loading branch information
@Haroenv @samouss
Haroenv authored and samouss committed Sep 17, 2019
1 parent d436639 commit 52d1e5c
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 338 deletions.
167 changes: 17 additions & 150 deletions src/lib/utils/__tests__/enhanceConfiguration-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('enhanceConfiguration', () => {
const widget = createWidget(configuration);

const output = enhanceConfiguration(configuration, widget);
expect(output).toEqual(configuration);
expect(output).toEqual(expect.objectContaining(configuration));
});

it('should call `getConfiguration` from widget correctly', () => {
Expand All @@ -48,174 +48,41 @@ describe('enhanceConfiguration', () => {
expect(output.analytics).toBe(true);
});

it('should deduplicate primitive array', () => {
it('should union facets', () => {
{
const actualConfiguration = { refinements: ['foo'] };
const widget = createWidget({ refinements: ['foo', 'bar'] });
const actualConfiguration = { facets: ['foo'] };
const widget = createWidget({ facets: ['foo', 'bar'] });

const output = enhanceConfiguration(actualConfiguration, widget);
expect(output.refinements).toEqual(['foo', 'bar']);
expect(output.facets).toEqual(['foo', 'bar']);
}

{
const actualConfiguration = { refinements: ['foo'] };
const widget = createWidget({ refinements: ['bar'] });
const actualConfiguration = { facets: ['foo'] };
const widget = createWidget({ facets: ['bar'] });

const output = enhanceConfiguration(actualConfiguration, widget);
expect(output.refinements).toEqual(['foo', 'bar']);
expect(output.facets).toEqual(['foo', 'bar']);
}

{
const actualConfiguration = { refinements: ['foo', 'bar'] };
const widget = createWidget({ refinements: [] });
const actualConfiguration = { facets: ['foo', 'bar'] };
const widget = createWidget({ facets: [] });

const output = enhanceConfiguration(actualConfiguration, widget);
expect(output.refinements).toEqual(['foo', 'bar']);
expect(output.facets).toEqual(['foo', 'bar']);
}
});

it('should replace nested values', () => {
it('should replace unknown deep values', () => {
const actualConfiguration = { refinements: { lvl1: ['foo'], lvl2: false } };
const widget = createWidget({ refinements: { lvl1: ['bar'], lvl2: true } });

const output = enhanceConfiguration(actualConfiguration, widget);
expect(output).toEqual({
refinements: { lvl1: ['foo', 'bar'], lvl2: true },
});
});

it('should add `hierarchicalFacets`', () => {
const actualConfiguration = {};

const widget = createWidget({
hierarchicalFacets: [
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
});

const output = enhanceConfiguration(actualConfiguration, widget);

expect(output).toEqual({
hierarchicalFacets: [
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
});
});

it('should add multiple `hierarchicalFacets`', () => {
const actualConfiguration = {
hierarchicalFacets: [
{
name: 'countries',
attributes: [
'countries.lvl0',
'countries.lvl1',
'countries.lvl2',
'countries.lvl3',
],
},
],
};

const widget = createWidget({
hierarchicalFacets: [
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
});

const output = enhanceConfiguration(actualConfiguration, widget);

expect(output).toEqual({
hierarchicalFacets: [
{
name: 'countries',
attributes: [
'countries.lvl0',
'countries.lvl1',
'countries.lvl2',
'countries.lvl3',
],
},
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
});
});

it('should deduplicate `hierarchicalFacets` with same name', () => {
const actualConfiguration = {
hierarchicalFacets: [
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
};

const widget = createWidget({
hierarchicalFacets: [
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
});

const output = enhanceConfiguration(actualConfiguration, widget);

expect(output).toEqual({
hierarchicalFacets: [
{
name: 'categories',
attributes: [
'categories.lvl0',
'categories.lvl1',
'categories.lvl2',
'categories.lvl3',
],
},
],
});
expect(output).toEqual(
expect.objectContaining({
refinements: { lvl1: ['bar'], lvl2: true },
})
);
});
});
116 changes: 0 additions & 116 deletions src/lib/utils/__tests__/mergeDeep-test.ts
View file

This file was deleted.

49 changes: 7 additions & 42 deletions src/lib/utils/enhanceConfiguration.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { PlainSearchParameters } from 'algoliasearch-helper';
import algoliasearchHelper, {
PlainSearchParameters,
} from 'algoliasearch-helper';
import { Widget } from '../../types';
import findIndex from './findIndex';
import mergeDeep from './mergeDeep';
import mergeSearchParameters from './mergeSearchParameters';

export function enhanceConfiguration(
configuration: PlainSearchParameters,
Expand All @@ -14,45 +15,9 @@ export function enhanceConfiguration(
// Get the relevant partial configuration asked by the widget
const partialConfiguration = widget.getConfiguration(configuration);

if (!partialConfiguration) {
return configuration;
}

if (!partialConfiguration.hierarchicalFacets) {
return mergeDeep(configuration, partialConfiguration);
}

const {
hierarchicalFacets,
...partialWithoutHierarchcialFacets
} = partialConfiguration;

// The `mergeDeep` function uses a `uniq` function under the hood, but the
// implementation does not support arrays of objects (we also had the issue
// with the Lodash version). The `hierarchicalFacets` attribute is an array
// of objects, which means that this attribute is never deduplicated. It
// becomes problematic when widgets are frequently added/removed, since the
// function `enhanceConfiguration` is called at each operation.
// https://github.com/algolia/instantsearch.js/issues/3278
const configurationWithHierarchicalFacets = {
...configuration,
hierarchicalFacets: hierarchicalFacets.reduce((facets, facet) => {
const index = findIndex(facets, _ => _.name === facet.name);

if (index === -1) {
return facets.concat(facet);
}

const nextFacets = facets.slice();
nextFacets.splice(index, 1, facet);

return nextFacets;
}, configuration.hierarchicalFacets || []),
};

return mergeDeep(
configurationWithHierarchicalFacets,
partialWithoutHierarchcialFacets
return mergeSearchParameters(
new algoliasearchHelper.SearchParameters(configuration),
new algoliasearchHelper.SearchParameters(partialConfiguration)
);
}

Expand Down
1 change: 0 additions & 1 deletion src/lib/utils/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export { default as isEqual } from './isEqual';
export { default as escape } from './escape';
export { default as find } from './find';
export { default as findIndex } from './findIndex';
export { default as mergeDeep } from './mergeDeep';
export { default as mergeSearchParameters } from './mergeSearchParameters';
export { default as resolveSearchParameters } from './resolveSearchParameters';
export { warning, deprecate } from './logger';
Expand Down
Loading

0 comments on commit 52d1e5c

Please sign in to comment.