-
Notifications
You must be signed in to change notification settings - Fork 80
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
[CHAT-1814] Updates to message search #677
Conversation
Size Change: +803 B (0%) Total Size: 230 kB
|
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.
LGTM 🌮
@nhannah could you also take a look?
Co-authored-by: Amin Mahboubi <[email protected]>
src/types.ts
Outdated
export type SearchOptions = LimitOffsetSearchOptions | NextSearchOptions; | ||
|
||
export type LimitOffsetSearchOptions = { | ||
limit?: number; | ||
offset?: number; | ||
}; | ||
|
||
export type NextSearchOptions = { | ||
limit?: number; | ||
next?: string; | ||
sort?: SearchMessageSort; | ||
}; |
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.
export type SearchOptions = LimitOffsetSearchOptions | NextSearchOptions; | |
export type LimitOffsetSearchOptions = { | |
limit?: number; | |
offset?: number; | |
}; | |
export type NextSearchOptions = { | |
limit?: number; | |
next?: string; | |
sort?: SearchMessageSort; | |
}; | |
export type LimitOffsetSearchOptions = { | |
limit?: number; | |
offset?: number; | |
}; | |
export type NextSearchOptions = { | |
limit?: number; | |
next?: string; | |
sort?: SearchMessageSort; | |
}; | |
export type SearchOptions = LimitOffsetSearchOptions | NextSearchOptions; |
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.
Defining before using ... makes it a little easier to read!!
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.
Also, do we really need separate LimitOffsetSearchOptions
and NextSearchOptions
? Because anyways user can't switch between v1 or v2 search, right?
export type SearchOptions = {
limit?: number;
next?: string;
sort?: SearchMessageSort;
};
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 thought it was better to keep them separate - A user should not be allowed to use offset with next or with sort.
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.
Ah my bad. I thought sort
was the only difference there :) Thanks for clarification
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.
Hey let's look at this together, if you have to hop in the TS world more I am happy to help, and we can add a test to give some piece of mind on these as well.
src/channel.ts
Outdated
const payload = { | ||
filter_conditions: { cid: this.cid }, | ||
...options, | ||
const { sort, ...optionsWithoutSort } = { ...options }; |
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.
So sort here is throwing a TS error, this is because only one of the 2 search options types has sort on it.
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.
Don't think you need to spread options here.
src/client.ts
Outdated
@@ -1779,7 +1779,7 @@ export class StreamChat< | |||
>, | |||
options: SearchOptions = {}, | |||
) { | |||
// Return a list of channels | |||
const { sort: sort_value, ...options_without_sort } = { ...options }; |
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.
Same as in channel but also we can probably avoid the rest spread as well, and pop everything to one line via
sort: sort &&= normalizeQuerySort(sort_value)
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 not quite sure what you want to do here. Which lines will be replaced with your suggestion?
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 put in some suggestions; looks like that old util func is messy as well so I put in a suggestion there that I think will correct it's off typing.
src/types.ts
Outdated
@@ -1911,6 +1947,10 @@ export type SearchPayload< | |||
UserType | |||
>; | |||
query?: string; | |||
sort?: Array<{ | |||
direction: AscDesc; | |||
field: Extract<keyof SearchMessageSortBase<MessageType>, string>; |
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.
Am I missing something here?
keyof SearchMessageSortBase<MessageType>
will always be a string, so Extract<keyof SearchMessageSortBase<MessageType>, string>
should be the same thing as keyof SearchMessageSortBase<MessageType>
? But maybe I am not seeing something?
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.
keyof SearchMessageSortBase<MessageType>
is string | number
Co-authored-by: Neil <[email protected]>
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.
Thanks for the changes, looks good!
CLA
Description of the changes, What, Why and How?
Updates the
channel.search()
andclient.search()
endpoints to support pagination using a next value and sorting parameters. For example:Changelog