Expose a set of Meilisearch search parameters to be overridden/configured #1258
Conversation
🦋 Changeset detectedLatest commit: 95d1f6e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -32,16 +33,17 @@ import { constructClientAgents } from './agents' | |||
|
|
|||
| /** | |||
| * Instantiate SearchClient required by instantsearch.js. | |||
There was a problem hiding this comment.
Perhaps this comment should be tweaked, to explain the new return type.
| @@ -214,19 +246,22 @@ export function adaptSearchParams( | |||
| ): MeiliSearchMultiSearchParams { | |||
| const meilisearchParams = MeiliParamsCreator(searchContext) | |||
| meilisearchParams.addQuery() | |||
| meilisearchParams.addIndexUid() | |||
There was a problem hiding this comment.
Because index uid is non-optional, I opted for stronger types, and always initiate meiliSearchParams with indexUid.
| @@ -20,7 +20,6 @@ export function getParametersWithoutFilters( | |||
| } | |||
| const meilisearchParams = MeiliParamsCreator(defaultSearchContext) | |||
| meilisearchParams.addFacets() | |||
| meilisearchParams.addIndexUid() | |||
There was a problem hiding this comment.
This always needs to be added, so the MeiliParamsCreator does it.
| export const enum MatchingStrategies { | ||
| ALL = 'all', | ||
| LAST = 'last', | ||
| } |
There was a problem hiding this comment.
This should be imported from Meilisearch JS package, and its type fixed there. (meilisearch/meilisearch-js#1588)
|
Seems like I've made a faulty merge. |
flevi29
force-pushed
the
expose-search-parameters
branch
from
3403fa6
to
56d5a46
Compare
|
Jetbrains IDE merge did some weird stuff, where merge did not delete anything, and only kept additions, or at least I believe that's what happened. I tried reverting, but just couldn't get it right so I had to reset and merge again. Apologies if this causes any problem with git history. |
|
Looks like I didn't run some of the tests. I only ran |
|
Hey @flevi29, I'm the current maintainer of the repo, and I have a lot on my plate right now. I'll take a look at your code ASAP :) Thanks for the contribution! |
There was a problem hiding this comment.
Hi @flevi29, I hope you're good!
Thanks a lot for your time doing this update; I'm sure the small breaking change will be paid off soon!
Unfortunately, I've lacked time in the last few months due to other company priorities, and reviewing a PR like this is very demanding to me now. So, I looked at it, and it is good from my POV.
Since this is an important change, and I want to ensure people can take advantage of it ASAP (the PR has been waiting for a long time now), I'm going to merge and release it.
Anyway, thanks for your effort @flevi29!
By the way, if anyone is seeing this comment, please step in regarding reviewing PRs or raising new PRs, we have a very engaged community, and we need you more than ever.
brunoocasali
added
enhancement
|
bors merge |
|
Hey @brunoocasali! No problem, take your time. I will keep contributing when I find the time, for my next issue this was kind of a blocking one. Thanks for the kind words, and godspeed with current priorities! |
Pull Request
Related issue
Fixes #1249
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!