This repository has been archived by the owner on Mar 21, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Define fields to search on at runtime #251
Define fields to search on at runtime #251
Changes from 8 commits
f5db7b6
705121c
6d22d58
97cea19
8bf29c6
eba4c93
12211c7
0f97847
015cfd1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What happens if you send an empty 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.
Empty result
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.
Should that be specified or is it common practice in the API?
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 that defining an empty attributesToSearchOn list means that you search in no attributes, which implies no results. Do you see any other behavior?
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 guess not, I was just wondering if it might be surprising for a user to leave it empty and then be confused about getting no results
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'm more on the side of the error if the array is sent with an empty form, so the developer can't break the search and is aware of it; By doing this, we can catch it before they have to understand what's wrong and fix it; I see no point in returning 0 results, am I wrong?
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.
Maybe we could return the hits as if the query was empty?
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 agree about the fact that would be useless to set
[]
, however, it's a logical behavior from Meilisearch in my sense. Moreover, it's consistent with thesearchableAttributes
that accepts an empty arrayIt doesn't seems logical to me to replace the search with a placeholder one when
attributeToSearchOn
is empty. 🤔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 right @ManyTheFish; accepting an empty array is an aspect of "consistency" that we could revisit with a v2 eventually because it doesn't seem great for DX (letting the developer put himself in a situation where he/she gets nothing out of the product)
I see it as an edge case personally, so the current implementation works for me, and I'm not against it
cc @macraig (if it helps!)
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.
+1 @gmourier , let's be consistent and revisit this when we're ready for a v2