-
Notifications
You must be signed in to change notification settings - Fork 99
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
add option to pass headers in the meilisearch client builder #721
base: main
Are you sure you want to change the base?
Conversation
You can pass a http client configured with your headers, as it's a http client responsibility 🤔 |
I get it, and that's how I am solving this issue right now. But the In fact in some cases, I would like to be able to change the user-id passed in the header on the go. Maybe with a It is obvious to me that few only projects will need to pass a "custom" http client to the MeilisearchClient, but all of them would like to take advantage of precise analytics. And therefore pass a "X-MS-USER-ID". So I find it interesting to make that task more "accessible". Maybe the right thing to do is not to touch the headers upfront, but to expose a method like I know analytics is in beta for now, so all of this is maybe new and not much used. But as a developer, not needing to customise my http client. I would prefer having a simple way to pass this info. I did not find any Analytics methods in the php doc neither. For example to report a clic on a search hit, or a conversion. Maybe I am missing a key concept here and not seeing the big picture, and maybe I am not expressing myself clearly enough because english is not my native language. So obviously feel free to jump in and explain why it would not be the way to go. Thanks |
I may have not been clear enough on the fact that I want to simplify the implementation of Obviously if I want to configure and use custom headers, the best practice would be to pass a custom http client. I get that adding an optional headers array, whilst seeming the easiest implementation on my end, is not the best suited for the long run. Maybe a method like |
I wouldn't say so. Stateful singleton is more harmful than instantiating single per user :) I see in docs it is explicitly related to search endpoint. So I think this could be accepted as a SearchQuery option, but not as a global setting for whole Meilisearch client |
Well as of for now, analytics is only done on searches, so yes indeed it would make more sense to add it as a Search Query option. And regarding other analytics endpoints ? Like hit clicking, conversions, etc ? Would be nice to implement those as well. |
Yes, for this you could open another issue :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #721 +/- ##
==========================================
- Coverage 90.74% 89.53% -1.21%
==========================================
Files 52 56 +4
Lines 1340 1424 +84
==========================================
+ Hits 1216 1275 +59
- Misses 124 149 +25 ☔ View full report in Codecov by Sentry. |
Pull Request
Related issue
Fixes #720
What does this PR do?
Improvement
For example :
new Meilisearch(MEILISEARCH_HOST, MEILISEARCH_SEARCH_KEY, null, null, [], null, ["X-MS-USER-ID" => example])
=> But it was the fastest solution and the less destructive.addHeader()
PR checklist
Please check if your PR fulfills the following requirements:
Extra info
First contribution of mine to this repo, maybe I missed some subtilities.