-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): introduce Requester API #540
Conversation
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 81d9ff7:
|
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.
looks great!
packages/autocomplete-preset-algolia/src/requester/getAlgoliaFacets.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-preset-algolia/src/requester/getAlgoliaResults.ts
Outdated
Show resolved
Hide resolved
Two followup PRs:
|
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.
without testing the detailed behaviour, the code definitely looks good and much clearer!
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.
Yet an other clean update 🎉
@@ -72,7 +72,7 @@ autocomplete<ProductHit>({ | |||
{ | |||
sourceId: 'products', | |||
getItems() { | |||
return getAlgoliaHits<ProductHit>({ | |||
return getAlgoliaResults<ProductRecord>({ |
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.
return getAlgoliaResults<ProductRecord>({ | |
return getAlgoliaResults({ |
I think this should stay a ProductHit
, otherwise the type returned by getSources
isn't right
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.
Fixed in #542 (just the function).
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.
I think the problem is that client.search
returns Hit<TRecord>
, so if we provide ProductHit
(and not ProductRecord
) to getAlgoliaResults
, it would return Hit<ProductHit>
(i.e., Hit<Hit<ProductRecord>>
). Perhaps that's not an issue since Hit
is idempotent, but not too sure how TypeScript handles those.
So providing ProductHit
indeed seems more correct, although then we're manipulating Hit<ProductHit>
everywhere else.
* docs: document Requester API functions * docs: update occurrences of former API * docs: fix example using former API * fix: apply suggestions from code review Co-authored-by: François Chalifour <[email protected]> * docs: add missing parameters * docs: fix example * docs: fix function name * feat: automatically set type when using getAlgoliaFacets (#543) * feat: introduce createGetAlgoliaFacets * feat: expose RequesterParams type * refactor: use factory to create function * test: update tests * docs: update docs * fix: update implems * build: increase bundle size * refactor: keep duplicates for simplicity * style: lint * fix: apply suggestions from code review Co-authored-by: François Chalifour <[email protected]> * test: better test getAlgoliaResults (#544) * test: remove unnecessary async/await keywords * test(autocomplete-js): add unit test for getAlgoliaResults * fix: remove needless async Co-authored-by: François Chalifour <[email protected]> * fix: update examples and tests with new API (#542) Co-authored-by: François Chalifour <[email protected]>
## Description In #540, we introduced the Requester API to batch Algolia queries, but we [left out Insights plugin support](https://github.com/algolia/autocomplete/blob/c62f1d2757c5824c5a2f456c20cf55ca3d962cdb/packages/autocomplete-preset-algolia/src/search/getAlgoliaHits.ts#L18-L19). These metadata fields are required for the Insights plugin to send events to the API. This PR brings support for the Insights plugin from requesters by adding back the necessary metadata in the hits returned by the Algolia API. ## Related - #559 (reply in thread)
Description
Autocomplete relied on the
getAlgoliaHits
function coming from@algolia/autocomplete-preset-algolia
to query Algolia indices. Since this function is used outside of any scoped container, we cannot control the requests sent to Algolia and to other backends.The idea of this Requester API is to take control over these search calls to transform them and batch them. Batching Algolia calls significantly reduces the amount of requests sent to Algolia, and therefore reduces the cost customers pay to use federated search experiences with Autocomplete.
Imagine you have the following sources in Autocomplete:
With the previous API, this would result in 3 network requests sent to Algolia at every key stroke. With this new Requester API, this results in a single network request because we're able to batch all queries (regular queries and facet queries too).
How
In terms of public API, only the requesters' names change:
getAlgoliaHits
→getAlgoliaResults
getAlgoliaFacetHits
→getAlgoliaFacets
The previous requesters were directly calling
searchClient.search()
, but they now just become descriptions (i.e., object that we're able to interpret internally).These descriptions are now understood by our internal resolving mechanism, that groups descriptions based on their
searchClient
and theirexecute
reference (execute
is a reference to an internalfetchAlgoliaResults
function which never changes user-land yet).This means that if multiple sources use the same
searchClient
identity, we'll group them together to send a single network requests and redistribute the results to their respective callers (identified with their source ID). It is now required thatsourceId
s are unique, we throw an error if they're not.Usage
Notice that we're not able to use
getAlgoliaResults(/* ... *).then()
anymore because it returns an object and not a promise. It's replaced by thetransformResponse
param.Changes summary
getAlgoliaHits
→getAlgoliaResults
getAlgoliaFacetHits
→getAlgoliaFacets
sourceId
s must be uniqueWhat's next
This is API is a drastic change to how our internal mechanism work, we plan to release this new API quite silently on a new
alpha
tag and ask current users to update their apps to report any wrong behavior.This PR is just the foundation of powerful requesters in Autocomplete, but we would like to leverage more features in the future: