Introduce finitePagination setting: remove default fetch of 200 documents #707

bidoubiwa · 2022-03-22T16:52:58Z

Fixes: #569
fixes: #521

Previously

By default instant-meilisearch used to fetch 200 documents on every search request. This was needed to ensure we can provide a finite pagination when the user used the pagination widget (with the 1, 2, 3, at the bottom of the screen).

Issue

This impacted the speed of the search considerably. Indeed meilisearch is designed to retrieve the most relevant documents, but not all the relevant documents. The more you ask, the more the search engine has to process. There is no difference regarding the search time between retrieving 6 or 7 documents, but between 6 and 200 documents to every request, the impact can be huge.

It also impacted users using the infiniteHits widget as well while it is not necessary for them. The infiniteHits settings does not need to showcase a start and an end to the pagination. It only has to enable the show more button whenever there are more hits after the current showcased ones.

New behavior

This PR introduces a new parameter: finitePagination.

finitePagination has a default value of false, when set to false, each search requests a limit: (hitsPerPage + 1) * (page + 1)

hitsPerPage (parameter of instantsearch and default to 6) is the number of results on each page, or added on the current page on each show more click in case of infiniteHits. In the case if infiniteHits, we need to be able to know if there are more hits to come to enable or disable the show more button. Thus we add 1 to hitsPerPage so that if Meilisearch returns hitsPerPage + 1 documents we know it has more.
page: behind the scene value tracking the current page the user is on. For example 1 if you clicked once on load more. Because it starts at 0, we add +1 for the multiplication.

The pagination will stop at paginationTotalHits (default 200).

When settings finitePagination to true, one search request will ask for a limit of paginationTotalHits as limit. Thus in the default case, one search request will return 200 documents.
This number can be changed with paginationTotalHits

The following will make one request of 7 documents as hitsPerPage is at 6 by default.

instantMeiliSearch(host, key, {
   finitePagination: false // default false
   paginationTotalHits: 60 // default 200
})

You can change hitsPerPage.

The following will make one request of 60 documents

instantMeiliSearch(host, key, {
   finitePagination: true // default false
   paginationTotalHits: 60 // default 200
})

alallema

LGTM! 🥗

curquiza · 2022-03-23T09:37:15Z

Hello! Awesome for this PR 🚀

⚠️ Disclaimer: my feedback can seem huge, I'm just picky to make you improve in writing PR descriptions. This one is a good opportunity since it really interests Quentin and Morgane for the mini-dashboard, so "external" people 🤘
But your PR is really cool, the code seems clear, and you added tests! 🔥 Thank you for this! Can't wait we upgrade the mini-dashboard with this improvement!

This impacted the speed of the search considerably.

Maybe remind the more you ask the search engine to retrieve documents, the more you impact the search time. Indeed meilisearch is designed to retrieve the most relevant documents, but not all the relevant documents. The more you ask, the more the search engine has to process. There is no difference regarding the search time between retrieving 6 or 7 documents, but between 6 and 200 documents to every request, the impact can be huge.
(I don't you already know this @bidoubiwa, just to help describe the context to someone going to your PR 😇)
Also, I know the context is kind of described in the issues, but

It's always appreciated to remind the context/motivation in the PR, what you started to do, and thank you for this!
it's not really convenient to check the issues related when there are multiple ones

Now by default:
Each search requests a limit of hitsPerPage (default: 6) and the page number : (hitsPerPage + 1) * (pageNumber + 1)
since pageNumber starts at 0 we need to add one (as 0 * x is always 0...). We also need to add one to hitsPerPage to ensure that there is still something to load after, as without we don't want load more to be clickable.

You mean, when finitePagination is set to false right?
I don't understand why there is pageNumber in the calculation formula of pageNumber, see: "Each search requests a limit of hitsPerPage (default: 6) and the page number : (hitsPerPage + 1) * (pageNumber + 1)". Is it recursive?
Do you mean by default only the next page was accessible? I think the context without the calculation is missing this PR description. Since the README is not updated, it's really missing.
Curisotity: why 6 for hitsPerPage?

The following will make one request of 7 documents as hitsPerPage is at 6 by default.
instantMeiliSearch(host, key, {
   finitePagination: false // default false
   paginationTotalHits: 60 // default 200
})
You can change hitsPerPage.

The following will make one request of 200 documents
instantMeiliSearch(host, key, {
   finitePagination: true // default false
   paginationTotalHits: 60 // default 200
})

The examples are clear 👌
I would say "The following will make instant-meilisearch request Meilisearch to retrieve 7 documents and not 6".

Finally, is it expected the README is not updated here? If it is, maybe you should add context in the PR description, like "the README will be updated in this PR", especially because this PR will be merged into main

Thank you again for the PR! ♥️

curquiza · 2022-03-23T10:17:31Z

Also, in the PR name, I would highlight the introduction of the finitePagination parameter :)

bidoubiwa · 2022-03-23T13:01:14Z

@curquiza I think I added all your suggestions. Can you confirm that the readme now appears in this PR?

Also, about the hitsPerPage it is a instantsearch setting and not a instant-meilisearch one.

curquiza

Good documentation and nice PR description 👌
Also pinging @gmourier only for your information (about pagination héhé)

⚠️ I did not check the code

bidoubiwa · 2022-03-28T11:00:10Z

bors merge

meili-bors · 2022-03-28T11:03:21Z

Build succeeded:

brunoocasali

Hi @bidoubiwa I left some comments about the fix, let me know if you need something :)

PS: I know I messed up with the timing, but I think is still helpful :)

brunoocasali · 2022-03-23T14:20:16Z

README.md

+
+### Finite Pagination
+
+Finite pagination is used when you want to add a numbered pagination at the bottom of your hits (for example: `< << 1, 2, 3 > >>`).


Suggested change

Finite pagination is used when you want to add a numbered pagination at the bottom of your hits (for example: `< << 1, 2, 3 > >>`).

Finite pagination is used when you want to add a numbered pagination at the bottom of your hits (for example: `< << 1, 2, 3 >> >`).

brunoocasali · 2022-03-23T14:22:15Z

jest.config.js

@@ -6,6 +6,15 @@ module.exports = {
    'jest-watch-typeahead/filename',
    'jest-watch-typeahead/testname',
  ],
+  collectCoverage: true,


Oh, I saw the coverage in the CI, you have good coverage in this project, congrats! 🎉 🌮

brunoocasali · 2022-03-28T19:52:29Z

tests/pagination.tests.ts

@@ -148,4 +148,57 @@ describe('Pagination browser test', () => {
    const hits = response.results[0].hits
    expect(hits.length).toBe(6)
  })
+


I did not know where I could add this, but is there a way to remove the "load more" and automate the "load more" request just by scrolling down?

If yes, last year I saw this video about how to make your app look better in infinite paginations. They have a trick with making two requests at once and I found it really great (despite being an ember tutorial, the concept is much valid) - but only if there is a way to automate the load more.

It is possible by tweaking and customizing the instantsearch.js see here

brunoocasali · 2022-03-28T19:53:52Z

src/types/types.ts

+  page: number
+}
+
+export type PaginationParams = {


You don't need this type right?

This one has all its parameters being optional, the other one none are optional. That way once I initialized PaginationContext using the information inPaginationParams I travel around with an object that has either a value provided by PaginartionParams or a default value

brunoocasali · 2022-03-28T20:06:48Z

src/adapter/search-request-adapter/search-params-adapter.ts

-  // Limit
-  if ((!placeholderSearch && query === '') || paginationTotalHits === 0) {
+  // Pagination
+  const { pagination } = searchContext


I was thinking of a way to reduce the complexity of this function, it seems this accumulated a lot of work in the last additions (and I have a feeling this will keep happening in the future 😬).

Maybe there is a way to apply the builder design pattern here, to build the meiliSearchParams object in a clear way (but in a functional way).

Another way could be create smaller functions which will handle the data as a immutable param and return a new param like with the new composition:

// meilisearchParams = {} meilisearchParams = addAttributesToRetrieve(meilisearchParams) // meilisearchParams is now {attributesToCrop: [...]} meilisearchParams = addAttributesToHighlight(meilisearchParams) // meilisearchParams is now {attributesToCrop: [...], attributesToHighlight: [...]}

You could notice that this function is doing a lot because we have comments separating the responsibilities of the function like pagination, Attributes To Retrieve, filters....

Another thing I would like to ask is in line 20:
const meiliSearchParams: Record<string, any> = {}, shouldn't be const meiliSearchParams: MeiliSearchParams = {}?

btw, I didn't find a good reference about a builder pattern in FP but looking at it, I found some good videos about FP in general

https://www.youtube.com/watch?v=v9QGWbGppis

https://www.youtube.com/watch?v=srQt1NAHYC0

const meiliSearchParams: Record<string, any> = {}, shouldn't be const meiliSearchParams: MeiliSearchParams = {}?

The issue lies with InstantSearchParams being read-only and MeiliSearchParams being not read-only. It results for example in

meiliSearchParams.facetsDistribution = searchContext?.facets

throwing:

Type 'readonly string[] | undefined' is not assignable to type 'string[] | undefined'. The type 'readonly string[]' is 'readonly' and cannot be assigned to the mutable type 'string[]'.t

But by saying Record<string, any> it removes this conflict. At the end, I still return a MeiliSearchParams so if I created the object incorrectly it will throw an error anyway.

For the builder pattern I think the adapter pattern is best suited! See #726

src/adapter/search-request-adapter/__tests__/search-params.tests.ts

tests/pagination.tests.ts

727: Cleaning in tests r=bidoubiwa a=bidoubiwa Based on these comments the tests are made more readable and consistent: - new lines between concerns - No redundant `Test` in front of test name - Creation of default contexts where possible - remove duplicated tests - Group related <hr> - #707 (comment) - #707 (comment) - #707 (comment) - #707 (comment) - #707 (comment) - #707 (comment) Co-authored-by: Charlotte Vermandel <charlottevermandel@gmail.com> Co-authored-by: cvermand <33010418+bidoubiwa@users.noreply.github.com>

bidoubiwa added 2 commits March 22, 2022 17:38

Add infinite pagination by default

6d53dc5

Add documentation for finite pagination

86e8114

bidoubiwa requested review from alallema and brunoocasali March 22, 2022 17:23

bidoubiwa added the breaking-change The related changes are breaking for the users label Mar 22, 2022

bidoubiwa requested a review from curquiza March 22, 2022 17:52

alallema previously approved these changes Mar 23, 2022

View reviewed changes

Improve variable naming

4f4b7ff

bidoubiwa dismissed alallema’s stale review via 4f4b7ff March 23, 2022 09:56

bidoubiwa changed the title ~~Remove default fetch of 200 documents.~~ Remove default fetch of 200 documents which can be enabled with finitePagination setting Mar 23, 2022

bidoubiwa changed the title ~~Remove default fetch of 200 documents which can be enabled with finitePagination setting~~ Introduce finitePagination setting: remove default fetch of 200 documents Mar 23, 2022

Improve documentation

a5d1614

bidoubiwa requested a review from alallema March 23, 2022 14:12

curquiza approved these changes Mar 24, 2022

View reviewed changes

bidoubiwa added the enhancement New feature or request label Mar 28, 2022

meili-bors bot merged commit 5446e6e into main Mar 28, 2022

meili-bors bot deleted the remove_default_finitesearch branch March 28, 2022 11:03

brunoocasali reviewed Mar 28, 2022

View reviewed changes

bidoubiwa mentioned this pull request Apr 4, 2022

Cleaning in tests #727

Merged

bidoubiwa removed the enhancement New feature or request label Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce finitePagination setting: remove default fetch of 200 documents #707

Introduce finitePagination setting: remove default fetch of 200 documents #707

bidoubiwa commented Mar 22, 2022 •

edited

Loading

alallema left a comment

curquiza commented Mar 23, 2022 •

edited

Loading

curquiza commented Mar 23, 2022

bidoubiwa commented Mar 23, 2022

curquiza left a comment •

edited

Loading

bidoubiwa commented Mar 28, 2022

meili-bors bot commented Mar 28, 2022

brunoocasali left a comment

brunoocasali Mar 23, 2022

brunoocasali Mar 23, 2022

brunoocasali Mar 28, 2022

bidoubiwa Apr 4, 2022

brunoocasali Mar 28, 2022

bidoubiwa Apr 4, 2022

brunoocasali Mar 28, 2022

brunoocasali Mar 28, 2022

bidoubiwa Apr 4, 2022

bidoubiwa Apr 4, 2022


		### Finite Pagination

		Finite pagination is used when you want to add a numbered pagination at the bottom of your hits (for example: `< << 1, 2, 3 > >>`).

Introduce finitePagination setting: remove default fetch of 200 documents #707

Introduce finitePagination setting: remove default fetch of 200 documents #707

Conversation

bidoubiwa commented Mar 22, 2022 • edited Loading

Previously

Issue

New behavior

alallema left a comment

Choose a reason for hiding this comment

curquiza commented Mar 23, 2022 • edited Loading

curquiza commented Mar 23, 2022

bidoubiwa commented Mar 23, 2022

curquiza left a comment • edited Loading

Choose a reason for hiding this comment

bidoubiwa commented Mar 28, 2022

meili-bors bot commented Mar 28, 2022

brunoocasali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidoubiwa commented Mar 22, 2022 •

edited

Loading

curquiza commented Mar 23, 2022 •

edited

Loading

curquiza left a comment •

edited

Loading