-
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: add recent-searches plugin #316
Conversation
d2be2c4
to
d157def
Compare
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 1444f02:
|
f32f5c1
to
9252f21
Compare
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.
Great!
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createRecentSearchStore.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-recent-searches/src/createRecentSearchesPlugin.ts
Outdated
Show resolved
Hide resolved
…earchesPlugin.ts Co-authored-by: François Chalifour <[email protected]>
/** | ||
* The array of plugins. | ||
*/ | ||
plugins?: Array<AutocompletePlugin<TItem, unknown>>; |
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.
This must be unknown
, right? We cannot know what it's going to be at this point. What do you think?
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.
Yep!
.then((sources) => | ||
sources.map((source) => ({ | ||
...source, | ||
onSelect: (payload) => { |
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.
We can use the term params
here too.
) | ||
.then((nested) => | ||
// same as `nested.flat()` | ||
nested.reduce((acc, array) => { |
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.
Why do we need to flatten sources here? (Does it reveal a data structure problem prior to this instruction?)
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.
No big deal, just to make the next iteration easier.
Co-authored-by: François Chalifour <[email protected]>
…e.js into feat/recent-searches
}, | ||
onSelect: ({ suggestion }) => { | ||
store.add(suggestion); | ||
const { query, objectID } = suggestion as any; | ||
if (query && objectID) { |
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.
This won't work with non-Algolia records so we should rather store the record and fallback objectID
to query
.
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.
@francoischalifour Okay, that part makes sense.
However I'm gonna have to leave if (query)
condition.
For example,
these products (not coming from suggestion index) don't have query
attribute.
In this case, is it correct that we're not storing them? Not sure how we've decided, but if it's correct, we need to keep if (query)
clause. cc @Shipow
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.
Perhaps this deserves a sync on Monday?
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.
If the source has getSuggestionUrl
, it doesn't reach to source.onSelect
, so it naturally doesn't get to be saved in recent searches.
While implementing this, I've realized that, for example,
-> then total 4 sources Whatever source triggers |
@francoischalifour what do you think? a1e5d98 |
} | ||
}, | ||
onSelect: ({ suggestion, state, source }) => { | ||
const inputValue = source.getInputValue({ suggestion, state }); |
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.
This is only going to work in the query-completion use case but let's scope it down to this and move forward 👍
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.
You're saying it won't work in case of "redirecting to a product page", right?
If so, yes. We're not thinking of the case in this PR: #316 (comment)
Summary
Here is the PoC of plugin API and the recent searches plugin on top of it.
This example is based on François' recent implementation.
Example →
These are all it takes to use the recent searches plugin.
Discussions
Batching API calls
From the perspective of
autocomplete-core
, it's impossible to know where a plugin is going to get sources from. If the core knows that a plugin is going to callgetAlgoliaResults
, then it can aggregate everything and fire a single request with multiple queries. We could change the implementation ofgetAlgoliaResults
to request to aBatchProcessor
(temp name), and when it's done, receives the result and return it. Anyway it looks like it's out of the scope of designing the plugin API.Customizable templates
With the current API, plugins can have
getSources
function which returns an array of sources. I haven't found any good way to force plugins to provide a specific API to customize templates. It's going to be up to each plugin to accept a certain option as they want.