nodejs / nodejs.org

The Node.js® Website
https://nodejs.org
MIT License
6.15k stars 6.22k forks source link

Implement debouncing in search #6717

Closed rahulkhattri0 closed 4 months ago

rahulkhattri0 commented 4 months ago

image

  1. Current behaviour is that every time the search text or the selected facet changes,we call the search api.
  2. This useEffect can be improved by adding basic debouncing.
mikeesto commented 4 months ago

Thanks for the issue. There is some history here as to why it is this way. Unless the Orama folk ask us to change it, I don't think it will be.

rahulkhattri0 commented 4 months ago

Ok I get it. Thanks for the explanation!

rahulkhattri0 commented 4 months ago

I guess a comment with this issue link can be added in the code.

rahulkhattri0 commented 4 months ago

Also, I feel this useEffect can be avoided because we just need to add the search logic in 2 event handlers, which is not a lot , IMO The two places where this search needs to be added are:

  1. onChange={event => setSearchTerm(event.target.value)} (in the search input)
  2. const changeFacet = (idx: string) => setSelectedFacet(Number(idx)); (where we change the facet) and moreover, using // eslint-disable-next-line react-hooks/exhaustive-deps is not really a good practice
rahulkhattri0 commented 4 months ago

@mikeesto please let me know what you think

ovflowd commented 4 months ago

Feel free to make a PR, @rahulkhattri0.

cc @micheleriva

micheleriva commented 4 months ago

Hi there, First of all, I strongly encourage everyone to use debounce. There's no real need, in my opinion... we are gifting unlimited searches to Node.js, and the front-end is not in trouble refreshing search results once every 100ms.

If you want to try debouncing search, you can use the Orama SDK itself:

const searchConfig = {
  debounce: 250
}

await orama.search({
  term: 'hello world',
}, searchConfig)

But again, I don't see a real reason why a debouncer is needed :)

rahulkhattri0 commented 4 months ago

@ovflowd ok so I'm removing the unnecessary useEffect and as @micheleriva is saying that that we don't need denouncing, so let it be that way.