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.

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

Merged
merged 19 commits into from
Jul 13, 2021
Merged

Conversation

miagilepner
Copy link
Contributor

@miagilepner miagilepner commented May 10, 2021

CLA

  • I have signed the Stream CLA (required).
  • Code changes are tested

Description of the changes, What, Why and How?

Updates the channel.search() and client.search() endpoints to support pagination using a next value and sorting parameters. For example:

// get a page of sorted results
const page1 = await client.search(
{ cid: my_cid }, 
'search text', 
{ limit: 100, sort: [{ relevance: -1 }, { updated_at: 1 }, { my_custom_field: -1 }]  });

// Query the second page of results. The same sort order will be used as in your first query, so there's no need to include it
const page2 = await client.search(
{ cid: my_cid }, 
'search text', 
{ limit: 100, next: page1.next });

// Pagination backwards by using the returned `previous` value
const page1Again = await client.search(
{ cid: my_cid }, 
'search text', 
{ limit: 100, next: page2.previous });

Changelog

@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2021

Size Change: +803 B (0%)

Total Size: 230 kB

Filename Size Change
dist/browser.es.js 50.1 kB +167 B (0%)
dist/browser.full-bundle.min.js 28.6 kB +143 B (0%)
dist/browser.js 50.7 kB +164 B (0%)
dist/index.es.js 50.1 kB +167 B (0%)
dist/index.js 50.7 kB +162 B (0%)

compressed-size-action

@miagilepner miagilepner changed the title [CHAT-1814] Message search v2 endpoint [CHAT-1814] Updates to message search May 28, 2021
Copy link
Contributor

@mahboubii mahboubii left a 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?

src/channel.ts Outdated Show resolved Hide resolved
@mahboubii mahboubii requested a review from nhannah July 5, 2021 15:30
miagilepner and others added 2 commits July 6, 2021 10:34
Co-authored-by: Amin Mahboubi <amin@getstream.io>
src/types.ts Outdated
Comment on lines 947 to 958
export type SearchOptions = LimitOffsetSearchOptions | NextSearchOptions;

export type LimitOffsetSearchOptions = {
limit?: number;
offset?: number;
};

export type NextSearchOptions = {
limit?: number;
next?: string;
sort?: SearchMessageSort;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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;

Copy link
Contributor

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!!

Copy link
Contributor

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;
};

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 thought it was better to keep them separate - A user should not be allowed to use offset with next or with sort.

Copy link
Contributor

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

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
vishalnarkhede
vishalnarkhede previously approved these changes Jul 7, 2021
Copy link
Contributor

@nhannah nhannah left a 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/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/channel.ts Outdated
const payload = {
filter_conditions: { cid: this.cid },
...options,
const { sort, ...optionsWithoutSort } = { ...options };
Copy link
Contributor

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.

Copy link
Contributor

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/channel.ts Outdated Show resolved Hide resolved
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 };
Copy link
Contributor

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)

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'm not quite sure what you want to do here. Which lines will be replaced with your suggestion?

Copy link
Contributor

@nhannah nhannah left a 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/channel.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/channel.ts Outdated Show resolved Hide resolved
src/channel.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/types.ts Outdated
@@ -1911,6 +1947,10 @@ export type SearchPayload<
UserType
>;
query?: string;
sort?: Array<{
direction: AscDesc;
field: Extract<keyof SearchMessageSortBase<MessageType>, string>;
Copy link
Contributor

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?

Copy link
Contributor Author

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

src/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nhannah nhannah left a 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!

@vishalnarkhede vishalnarkhede merged commit 1401bd9 into master Jul 13, 2021
@vishalnarkhede vishalnarkhede deleted the feat/messages-es branch July 13, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants