Add SearchResult.forEachHit

[Aschen]

What does this PR do?

Adds a SearchResult.forEachHit method that take a callback action and execute it over every hit of the search.
This method iterate over the page of results with the next() method and call the provided action of every elements of the results.hits array.

When we use the document:search method, a very current pattern is to apply an action on every results of the search whatever the number of pages.

while (results) {
  for (const hit of results.hits) {
    /* do something with hit */
  }
  results = await results.next()
}

Doc: kuzzleio/documentation#281

How should this be manually tested?

• Step 1 : Do some paginated search
• Step 2 : use results.forEachHit(hit => console.log(hit._id))
• Step 3 :

[codecov-io]

Codecov Report

Merging #386 into 6-dev will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            6-dev     #386      +/-   ##
==========================================
+ Coverage   96.68%   96.71%   +0.02%     
==========================================
  Files          30       30              
  Lines        1447     1460      +13     
==========================================
+ Hits         1399     1412      +13     
  Misses         48       48

Impacted Files	Coverage Δ
src/controllers/searchResult/base.js	95.52% <100%> (+1.07%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 717ce80...3303b76. Read the comment docs.

[benoitvidis]

Don't we support node 6?

[Aschen]

Node 6 LTS support ends on the 30/04/2019, shall we continue to support it anyway?

[ballinette]

why do we need to instantiate a new SearchResult object here ?

[Aschen]

The next method does not return a new SearchResult but instead it modify his properties.
I instantiate a new SearchResult to avoid mutating the original one when calling forEachHit

[scottinet]

I agree, the next method should return a new SearchResult instance, as it is done in other SDK, instead of mutating the current object.

[ballinette]

OK, so it's next method that should return a new instance.
Instead of a complex recursive code with firstCall, I'd rather fix the next method first.

Once done, you will only need to loop into this.hits and then call this.next().forEachHit(actions)

[scottinet]

I disagree with that feature entirely.

First, about the implementation: it gives the impression that the callback will be executed sequentially, but you're batch-processing it (asynchronous for each hit of a page, and each page sequentially). This is confusing ans misleading.

Second, I don't see what's wrong with the pattern it's supposed to replace: it's simple, it's clear, and it's short.

Third, I don't know how to properly document that feature. The only way to do it is to justify its existence by mentioning the pattern it aims to replace, and that doesn't feel enough to me.

This feels like a lot of code for a marginal gain to me.

[Aschen]

1. Yes I agree that the implementation can be confusing, I can change it to execute all the callback sequentially or even left the choice for the user.

2. Even if the pattern is simple, clear and short, it is widely used when you develop application with the SDK. Most of the time you only want to retrieve all the results in a single array or execute an API call for each results. IMHO this method is a way to DRY.

3. Not necessarily, IMHO even the name is explicit but I may be wrong.

About the code, for me we are developing theses SDK precisely to help users to develop application faster.
Dealing with search results is something so common that we even make a specific class for this. We already have the next method with provide a significant gain but even if the gain is marginal with forEachHit, it's a very common pattern and we could provide something to achieve DRY.
And since we have unit test on this feature (I can also add functional test), I didn't see what is the problem to have more code in the SDK.

[scottinet]

I can't help but think that all your feature does, is to get rid of one perfectly concise and clear do...while loop. And I find that underwhelming.

Going down that route, why not also add mapHits, filterHits, and other methods from the Array prototype?

Anyway, we'll need the team to review that one, because I don't think we can solve this disagreement by ourselves.

edit: and you can't compare this feature with next, which is a lot more complex since it has to handle different pagination strategies depending on how the initial search has been performed (scroll, search_after or from/size)