issue: https://linear.app/comm/issue/ENG-3167/implement-fetching-search-results-in-chunks-on-native
Adding a hook for fetching search results based on a query and a cursor.
Details
Details
- Reviewers
• kuba bartek kamil ashoat - Commits
- rCOMMe7c05100d749: [lib] Add useSearchMessages
Called the hook, cheked that it return correct results
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
(I think we should do this in a callback rather than an effect, but I think I mentioned that in another diff where I requested changes)
Comment Actions
(I think we should do this in a callback rather than an effect, but I think I mentioned that in another diff where I requested changes)
You mentioned in D7774 that I should not call something else from an effect.
I don't really understand how I could change this effect to a callback. If I do it like in ex useEditMessage, and return a return React.useCallback, it will still have to be called inside of an effect in the calling code, because I need to fetch messages whenever query or threadID or cursor change.
Comment Actions
You mentioned in D7774 that I should not call something else from an effect.
I don't really understand how I could change this effect to a callback. If I do it like in ex useEditMessage, and return a return React.useCallback, it will still have to be called inside of an effect in the calling code, because I need to fetch messages whenever query or threadID or cursor change.
Thanks @inka, I think you're right... I didn't consider that. We'll need to have an effect here.
| lib/shared/search-utils.js | ||
|---|---|---|
| 186 ↗ | (On Diff #26731) | It never makes sense to have an optional parameter before a required one. Please avoid this going forward! |
| 187 ↗ | (On Diff #26731) | Can you give this a better name? Maybe onResultsReceived? |
| 211–216 ↗ | (On Diff #26731) | This is significantly improved! I'm still a little concerned about the separation of logic between D7771 / D7774 / D7775. We have three files that are managing the state here, which is confusing and hard to follow. As an example, resetting the search results to empty when the query changes is handled in MessageSearch in D7774. But since your code here will re-query for results if eg. callSearchMessages or dispatchActionPromise changes, we should arguably be resetting the search results to empty in those cases too. Luckily those don't change often (or at all?), but it feels like the effect here is tightly coupled with the effect in MessageSearch, and if we added a new parameter to the dep list here, we might need to update the effect in MessageSearch. For that reason, I wonder if it would make sense to have these effects in the same file, right next to each other. Perhaps when you said "This diff will need to be dropped if those changes are accepted." in D7775, you were saying your plan is to unify some of this logic? |
| lib/shared/search-utils.js | ||
|---|---|---|
| 211–216 ↗ | (On Diff #26731) |
Three files? After these changes there are only two - search-utils and message-search. Do you mean the context?
I will return a callback from here and move the effect to MessageSearch. This should fix the issue. The callback will depend on callSearchMessages and dispatchActionPromise, and I will have the effect that clears the results depend on this callback. |
| lib/shared/search-utils.js | ||
|---|---|---|
| 189 ↗ | (On Diff #26849) | We should always be flexible with input parameters |