feat: add SearchController #1434
Conversation
MartinCupela
requested review from
vishalnarkhede,
santhoshvai,
khushal87,
szuperaz,
oliverlaz,
arnautov-anton and
myandrienko
as code owners
|
Size Change: +19.3 kB (+4.13%) Total Size: 486 kB
|
| hasMore: boolean; | ||
| isActive: boolean; | ||
| isLoading: boolean; | ||
| items: T[] | undefined; |
There was a problem hiding this comment.
| items: T[] | undefined; | |
| items: T[] | undefined; |
Why undefined? Can't it be an empty array instead?
There was a problem hiding this comment.
The value undefined says that no search has been executed yet. This is handy to know if we want to differ btw the state where will tell the user to start search vs telling the user there are not results for the given search query.
We could think that maybe the same could be inferred from the combination of the length of the search query and the length of the items array. The problem is, when you start typing, for a split of a second, the value of loading is not set. In that case you have a search query of non-zero lenght, loading is false and results are an empty array. That leads to showing "No results found" or whatever the empty results component is. So I would say we need an indicator that tells us, whether the items value is the result of a search query.
src/search_controller.ts
Outdated
| if (this.searchQuery) { | ||
| this.search(); | ||
| } |
There was a problem hiding this comment.
Maybe it'd be better to hide this side-effect behind a state-chage subscription of sorts or is it something you'd want to avoid in this case?
There was a problem hiding this comment.
Yes, I will remove the side effect from both the source and controller activate methods.
| this.searchDebounced = debounce(this.executeQuery.bind(this), debounceMs); | ||
| }; | ||
|
|
||
| activate = (sourceStateOverride?: Partial<SearchSourceState>) => { |
There was a problem hiding this comment.
Why the partial state here? I'd say activate should flip active to true and all the other side-effects should happen elsewhere. Multiple StateStore.next calls seem better than unexpected state changes seemingly unrelated to the method call.
| if (hasNewSearchQuery) { | ||
| this.resetState({ isActive: this.isActive, isLoading: true, searchQuery: newSearchString }); |
There was a problem hiding this comment.
To keep the behavior similar across the newly released classes, resetState shouldn't take any value but rather set the store to its initial state (at class construction). Any other modification should happen after the fact:
| if (hasNewSearchQuery) { | |
| this.resetState({ isActive: this.isActive, isLoading: true, searchQuery: newSearchString }); | |
| if (hasNewSearchQuery) { | |
| this.resetState(); | |
| this.state.partialNext({ isLoading: true, searchQuery: newSearchString }) |
There was a problem hiding this comment.
Or if you want to avoid two calls:
if (hasNewSearchQuery) {
this.state.next((current) => {
...current,
...this.initialState,
// overrides
isActive: current.isActive,
isLoading: true,
searchQuery: newSearchString
});| stateUpdate.lastQueryError = e as Error; | ||
| } finally { | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| this.state.next(({ lastQueryError, ...prev }: SearchSourceState<T>) => ({ |
There was a problem hiding this comment.
| this.state.next(({ lastQueryError, ...prev }: SearchSourceState<T>) => ({ | |
| this.state.next((current) => ({ |
Also, stateUpdate seems to get unwrapped after the "prev" (current state) and unless it (stateUpdate) contains reset value it does not really seem necessary to exclude it (reset value is not being set in the try block).
| ...prev, | ||
| ...stateUpdate, | ||
| isLoading: false, | ||
| items: [...(prev.items ?? []), ...(stateUpdate.items || [])], |
There was a problem hiding this comment.
Eliminating the undefined from T[] | undefined would seemingly reduce the unnecessary complexity here.
| this.state.next({ | ||
| hasMore: true, | ||
| isActive: false, | ||
| isLoading: false, | ||
| items: undefined, | ||
| lastQueryError: undefined, | ||
| next: undefined, | ||
| offset: 0, | ||
| searchQuery: '', | ||
| ...stateOverrides, | ||
| }); |
There was a problem hiding this comment.
Where initialState would get set at the beginning (during construction) and would be private readonly property.
| this.state.next({ | |
| hasMore: true, | |
| isActive: false, | |
| isLoading: false, | |
| items: undefined, | |
| lastQueryError: undefined, | |
| next: undefined, | |
| offset: 0, | |
| searchQuery: '', | |
| ...stateOverrides, | |
| }); | |
| this.state.next(this.initialState) |
| clear = () => { | ||
| this.cancelSearchQueries(); | ||
| this.sources.forEach((source) => source.resetState({ isActive: source.isActive })); | ||
| this.state.next((prev) => ({ |
There was a problem hiding this comment.
Nit:
| this.state.next((prev) => ({ | |
| this.state.next((current) => ({ |
| const { debounceMs, isActive, pageSize } = { ...DEFAULT_SEARCH_SOURCE_OPTIONS, ...options }; | ||
| this.pageSize = pageSize; | ||
| this.state = new StateStore<SearchSourceState<T>>({ | ||
| hasMore: true, |
There was a problem hiding this comment.
| hasMore: true, | |
| hasNext: true, |
Description of the changes, What, Why and How?
This is an addition of a new classes that control the search logic.
Advantages: