Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(core): integrate with Text Search API ordering #6001

Merged
merged 2 commits into from
Mar 19, 2024
Merged

feat(core): integrate with Text Search API ordering #6001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38dabf0
fix(core): reset global search results after ordering changes
juice49 Mar 14, 2024
cdde113
feat(core): finalise Text Search API ordering integration
juice49 Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions packages/sanity/src/core/search/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,10 @@ export interface TextSearchDocumentTypeConfiguration {
/**
* @internal
*/
export type TextSearchSort = Record<string, {order: SortDirection}>
export interface TextSearchOrder {
attribute: string
direction: SortDirection
ricokahler marked this conversation as resolved.
Show resolved Hide resolved
}

export type TextSearchParams = {
query: {
Expand Down Expand Up @@ -181,9 +184,9 @@ export type TextSearchParams = {
*/
types?: Record<string, TextSearchDocumentTypeConfiguration>
/**
* Result sorting.
* Result ordering.
*/
sort?: TextSearchSort[]
order?: TextSearchOrder[]
}

export type TextSearchResponse<Attributes = Record<string, unknown>> = {
Expand Down
14 changes: 6 additions & 8 deletions packages/sanity/src/core/search/text-search/createTextSearch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {describe, expect, it} from '@jest/globals'
import {Schema} from '@sanity/schema'
import {defineField, defineType} from '@sanity/types'

import {getDocumentTypeConfiguration, getSort} from './createTextSearch'
import {getDocumentTypeConfiguration, getOrder} from './createTextSearch'

const testType = Schema.compile({
types: [
Expand Down Expand Up @@ -201,7 +201,7 @@ describe('getDocumentTypeConfiguration', () => {
describe('getSort', () => {
it('transforms Studio sort options to valid Text Search sort options', () => {
expect(
getSort([
getOrder([
{
field: 'title',
direction: 'desc',
Expand All @@ -213,14 +213,12 @@ describe('getSort', () => {
]),
).toEqual([
{
title: {
order: 'desc',
},
attribute: 'title',
direction: 'desc',
},
{
_createdAt: {
order: 'asc',
},
attribute: '_createdAt',
direction: 'asc',
},
])
})
Expand Down
14 changes: 6 additions & 8 deletions packages/sanity/src/core/search/text-search/createTextSearch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import {
type SearchStrategyFactory,
type SearchTerms,
type TextSearchDocumentTypeConfiguration,
type TextSearchOrder,
type TextSearchParams,
type TextSearchResponse,
type TextSearchResults,
type TextSearchSort,
} from '../common'

const DEFAULT_LIMIT = 1000
Expand Down Expand Up @@ -73,12 +73,11 @@ export function getDocumentTypeConfiguration(
}, {})
}

export function getSort(sort: SearchSort[] = []): TextSearchSort[] {
return sort.map<TextSearchSort>(
export function getOrder(sort: SearchSort[] = []): TextSearchOrder[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😇

Copy link
Contributor Author

@juice49 juice49 Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return sort.map<TextSearchOrder>(
({field, direction}) => ({
[field]: {
order: direction,
},
attribute: field,
direction,
}),
{},
)
Expand Down Expand Up @@ -114,8 +113,7 @@ export const createTextSearch: SearchStrategyFactory<TextSearchResults> = (
...searchTerms.params,
},
types: getDocumentTypeConfiguration(searchOptions, searchTerms),
// TODO: `sort` is not supported by the Text Search API yet.
// sort: getSort(searchOptions.sort),
order: getOrder(searchOptions.sort),
includeAttributes: ['_id', '_type'],
fromCursor: searchOptions.cursor,
limit: searchOptions.limit ?? DEFAULT_LIMIT,
Expand Down
105 changes: 105 additions & 0 deletions packages/sanity/src/core/studio/components/navbar/search/contexts/search/reducer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,3 +285,108 @@ Array [
]
`)
})

it('should reset results after ordering changes', () => {
const {result} = renderHook(() => useReducer(searchReducer, initialState))
const [, dispatch] = result.current

act(() =>
dispatch({
type: 'TERMS_QUERY_SET',
query: 'test query a',
}),
)

act(() =>
dispatch({
type: 'SEARCH_REQUEST_COMPLETE',
nextCursor: 'cursorA',
hits: [
{
hit: {
_type: 'person',
_id: 'personA',
},
},
{
hit: {
_type: 'person',
_id: 'personB',
},
},
],
}),
)

const [stateA] = result.current

expect(stateA.result.hits).toMatchInlineSnapshot(`
Array [
Object {
"hit": Object {
"_id": "personA",
"_type": "person",
},
},
Object {
"hit": Object {
"_id": "personB",
"_type": "person",
},
},
]
`)

act(() =>
dispatch({
type: 'ORDERING_SET',
ordering: {
titleKey: 'search.ordering.test-label',
sort: {
field: '_createdAt',
direction: 'desc',
},
},
}),
)

act(() =>
dispatch({
type: 'SEARCH_REQUEST_COMPLETE',
nextCursor: undefined,
hits: [
{
hit: {
_type: 'person',
_id: 'personB',
},
},
{
hit: {
_type: 'person',
_id: 'personC',
},
},
],
}),
)

const [stateB] = result.current

expect(stateB.result.hits).toMatchInlineSnapshot(`
Array [
Object {
"hit": Object {
"_id": "personB",
"_type": "person",
},
},
Object {
"hit": Object {
"_id": "personC",
"_type": "person",
},
},
]
`)
})
4 changes: 4 additions & 0 deletions packages/sanity/src/core/studio/components/navbar/search/contexts/search/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ export function searchReducer(state: SearchReducerState, action: SearchAction):
...state,
ordering: action.ordering,
terms: stripRecent(state.terms),
result: {
...state.result,
hasLocal: false,
},
}
case 'PAGE_INCREMENT':
return {
Expand Down
Loading