feat(core): integrate with Text Search API ordering #6001
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
No changes to documentation |
|
Component Testing Report Updated Mar 14, 2024 4:57 PM (UTC)
|
| @@ -73,12 +73,11 @@ export function getDocumentTypeConfiguration( | |||
| }, {}) | |||
| } | |||
|
|
|||
| export function getSort(sort: SearchSort[] = []): TextSearchSort[] { | |||
| return sort.map<TextSearchSort>( | |||
| export function getOrder(sort: SearchSort[] = []): TextSearchOrder[] { | |||
There was a problem hiding this comment.
Now that the shape of the ordering lines up better with the params we send to the text search API, would it be better to remove this function and inline the logic?
There was a problem hiding this comment.
I don't see any strong reason to do so, but I'm happy to follow your advice 🙂. One advantage to getOrder being an individual function is that we can export it in order to unit test it (which we do).
I've followed this same style for the prefix search integration (#6010), which is unit tested in the same way.
What do you think?
There was a problem hiding this comment.
My preference for inline-it vs abstraction boils down to how complex the abstraction is. My preference is to inline it if I find it easier to understand what's going on by reading the contents of the abstraction rather than reading the name of the function. This is more of an art than a science for sure.
Regarding testing, I prefer to unit test at a higher level if possible and try to preserve the behavior at that level. In this case, I would test the createTextSearch function as a whole and mock out client.observable.request and capture the params that were sent to it and expect it to match a certain value vs testing the smaller parts of createTextSearch.
I kinda subscribe to the philosophy "write tests, not too many, mostly integration". I like this because if the implementation details change, the tests do not. However, this comes at the cost of the test itself being harder to write and requires more mocking.
At the end of the day this is an opinion and I'm not very dogmatic so feel free to merge 😇
There was a problem hiding this comment.
Thank you for explaining 🙏.
My preference for inline-it vs abstraction boils down to how complex the abstraction is.
That's a great point. There's a (somewhat fuzzy) threshold of complexity at which the cognitive cost of context switching to the abstraction becomes lesser than the cost of reading the implementation inline. I think you're probably right about the threshold not being met in this case.
In this case, I would test the createTextSearch function as a whole
Another great point. I really love testing small units precisely because the tests are simpler to write. createTextSearch isn't tested at the moment—it probably should be—but we'd then be testing the same implementation at two different levels. That's not necessarily a problem, but I think you have a good point that it'd be better to simply invest in testing createTextSearch itself in the first place.
At the end of the day this is an opinion and I'm not very dogmatic so feel free to merge 😇
I think merging is probably the pragmatic thing to do, only because I'm not sure we get a lot of value from changing our approach at this stage. However, I do think these are great points, and they'll certainly shape how I work in the future 🙂. Depending on the time we have available, I may make tweaks here and request another review.
juice49
requested review from
a team and
hermanwikner
and removed request for
a team
Description
This branch finalises the Text Search API ordering integration. Ordering is now formatted correctly and is included in Text Search API requests.
Note: The Text Search API functionality to support this has not yet been deployed to production.
It also includes a fix for global search results not resetting after ordering has changed. We didn't catch this before because ordering wasn't working at all.
What to review
Does ordering results from global search and document lists work as expected?
Testing
The
packages/sanity/src/core/search/text-search/createTextSearch.test.tstest suite ensures ordering requests are formatted correctly.