Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
},
{
"path": "packages/autocomplete-js/dist/umd/index.production.js",
"maxSize": "16.5 kB"
"maxSize": "16.75 kB"
},
{
"path": "packages/autocomplete-preset-algolia/dist/umd/index.production.js",
Expand Down
8 changes: 5 additions & 3 deletions packages/autocomplete-core/src/resolve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,13 @@ function isRequesterDescription<TItem extends BaseItem>(
type PackedDescription<TItem extends BaseItem> = {
searchClient: SearchClient;
execute: Execute<TItem>;
requesterId?: string;
items: RequestDescriptionPreResolved<TItem>['requests'];
};

type RequestDescriptionPreResolved<TItem extends BaseItem> = Pick<
RequesterDescription<TItem>,
'execute' | 'searchClient' | 'transformResponse'
'execute' | 'requesterId' | 'searchClient' | 'transformResponse'
> & {
requests: Array<{
query: MultipleQueriesQuery;
Expand Down Expand Up @@ -90,15 +91,15 @@ export function resolve<TItem extends BaseItem>(
return acc;
}

const { searchClient, execute, requests } = current;
const { searchClient, execute, requesterId, requests } = current;

const container = acc.find<PackedDescription<TItem>>(
(item): item is PackedDescription<TItem> => {
return (
isDescription(current) &&
isDescription(item) &&
item.searchClient === searchClient &&
item.execute === execute
item.requesterId === requesterId
Comment thread
Haroenv marked this conversation as resolved.
);
}
);
Expand All @@ -108,6 +109,7 @@ export function resolve<TItem extends BaseItem>(
} else {
const request: PackedDescription<TItem> = {
execute,
requesterId,
items: requests,
searchClient,
};
Expand Down
91 changes: 91 additions & 0 deletions packages/autocomplete-js/src/__tests__/requester.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import {
createRequester,
fetchAlgoliaResults,
} from '@algolia/autocomplete-preset-algolia';
import { fireEvent, waitFor, within } from '@testing-library/dom';

import {
Expand Down Expand Up @@ -346,6 +350,93 @@ describe('requester', () => {
});
});

test('batches calls when possible and re-dispatches results to the right sources across requester instances', async () => {
Comment thread
Haroenv marked this conversation as resolved.
Outdated
const container = document.createElement('div');
const panelContainer = document.createElement('div');

document.body.appendChild(panelContainer);

const searchClient = createSearchClient({
search: jest.fn(() =>
Promise.resolve(
createMultiSearchResponse<{ label: string }>(
{ hits: [{ objectID: '1', label: 'Hit 1' }] },
{ hits: [{ objectID: '2', label: 'Hit 2' }] }
)
)
),
});

const getResults1 = (params) =>
createRequester(fetchAlgoliaResults)({
transformResponse: (response) => response.hits,
})(params);

const getResults2 = (params) =>
createRequester(fetchAlgoliaResults)({
transformResponse: (response) => response.hits,
})(params);
Comment thread
Haroenv marked this conversation as resolved.

autocomplete({
container,
panelContainer,
getSources({ query }) {
return [
{
sourceId: 'hits',
getItems() {
return getResults1({
searchClient,
queries: [
{
indexName: 'indexName',
query,
},
],
});
},
templates: {
item({ item }) {
return JSON.stringify(item);
},
},
},
{
sourceId: 'other-hits',
getItems() {
return getResults2({
searchClient,
queries: [
{
indexName: 'indexName',
query,
},
],
});
},
templates: {
item({ item }) {
return JSON.stringify(item);
},
},
},
];
},
});

const input = container.querySelector<HTMLInputElement>('.aa-Input');

fireEvent.input(input, { target: { value: 'a' } });

await waitFor(() => {
expect(
panelContainer.querySelector<HTMLElement>('.aa-Panel')
).toBeInTheDocument();

expect(searchClient.search).toHaveBeenCalledTimes(1);
Copy link
Copy Markdown
Contributor

@tkrugg tkrugg Feb 22, 2022

Choose a reason for hiding this comment

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

nice test!

Not necessary now but I'd love to see one more test which I think would have prevented this issue: An E2E test using our UMD builds and counting requests.

Overall, having a test suite counting queries as we make changes would make a lot a sense, given the immediate impact on pricing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree on the end-to-end test. I believe this was initially in the PR then removed?

I recall our end-to-end setup not to be optimal, but this is a good opportunity to fix it given that one underlying issue is how we expect all our builds to work similarly (and in this case, they don't).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

there was an UMD E2E test initially indeed, but it wasn't actually easily testing for the case we were fixing. I'll recreate it in a separate PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the record, FX-1228 in the backlog is about this.

});
});

test('transforms the response before forwarding it to the state', async () => {
const container = document.createElement('div');
const panelContainer = document.createElement('div');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('getAlgoliaFacets', () => {

expect(description).toEqual({
execute: expect.any(Function),
requesterId: 'algolia',
transformResponse: expect.any(Function),
searchClient,
queries: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('getAlgoliaResults', () => {

expect(description).toEqual({
execute: expect.any(Function),
requesterId: 'algolia',
transformResponse: expect.any(Function),
searchClient,
queries: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import {

import { userAgents } from '../userAgents';

export const createAlgoliaRequester = createRequester((params) =>
fetchAlgoliaResults({
...params,
userAgents,
})
export const createAlgoliaRequester = createRequester(
(params) =>
fetchAlgoliaResults({
...params,
userAgents,
}),
'algolia'
);
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ describe('createRequester', () => {

expect(description).toEqual({
execute: expect.any(Function),
requesterId: undefined,
transformResponse,
searchClient,
queries: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('getAlgoliaFacets', () => {

expect(description).toEqual({
execute: expect.any(Function),
requesterId: 'algolia',
transformResponse: expect.any(Function),
searchClient,
queries: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('getAlgoliaResults', () => {

expect(description).toEqual({
execute: expect.any(Function),
requesterId: 'algolia',
transformResponse: expect.any(Function),
searchClient,
queries: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,7 @@ import { fetchAlgoliaResults } from '../search';

import { createRequester } from './createRequester';

export const createAlgoliaRequester = createRequester(fetchAlgoliaResults);
export const createAlgoliaRequester = createRequester(
fetchAlgoliaResults,
'algolia'
);
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,29 @@ export type RequestParams<THit> = FetcherParams & {
};

export type RequesterDescription<THit> = {
/**
* The search client used for this request. Multiple queries with the same client will be batched.
Comment thread
Haroenv marked this conversation as resolved.
Outdated
*/
searchClient: SearchClient;
/**
* Used to make sure queries should be batched. The Algolia requester uses "algolia" for this.
Comment thread
Haroenv marked this conversation as resolved.
Outdated
*/
requesterId?: string;
/**
* The search parameters used for this query
Comment thread
Haroenv marked this conversation as resolved.
Outdated
*/
queries: MultipleQueriesQuery[];
/**
* Change the response of this search before returning it to the caller
Comment thread
Haroenv marked this conversation as resolved.
Outdated
*/
transformResponse: TransformResponse<THit>;
/**
* Batched multi-query function. Is only called for one of the batched calls
Comment thread
Haroenv marked this conversation as resolved.
Outdated
*/
execute: Execute<THit>;
};

export function createRequester(fetcher: Fetcher) {
export function createRequester(fetcher: Fetcher, requesterId?: string) {
function execute<THit>(fetcherParams: ExecuteParams<THit>) {
return fetcher<THit>({
searchClient: fetcherParams.searchClient,
Expand All @@ -108,6 +124,7 @@ export function createRequester(fetcher: Fetcher) {
requestParams: RequestParams<TTHit>
): RequesterDescription<TTHit> {
return {
requesterId,
execute,
...requesterParams,
...requestParams,
Expand Down