Page MenuHomePhabricator

[lib] Add useSearchMessages
ClosedPublic

Authored by inka on May 10 2023, 3:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 23, 11:49 AM
Unknown Object (File)
Sun, May 19, 2:20 PM
Unknown Object (File)
Mon, May 6, 12:50 PM
Unknown Object (File)
Mon, May 6, 2:04 AM
Unknown Object (File)
Sun, May 5, 9:39 AM
Unknown Object (File)
Thu, May 2, 11:57 PM
Unknown Object (File)
Thu, May 2, 1:11 PM
Unknown Object (File)
Thu, May 2, 10:25 AM
Subscribers

Details

Summary

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.

Test Plan

Called the hook, cheked that it return correct results

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.May 10 2023, 3:48 AM
This revision is now accepted and ready to land.May 17 2023, 10:03 PM

(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)

First changes to address review on D7774

(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.

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?

inka planned changes to this revision.May 23 2023, 1:38 AM
lib/shared/search-utils.js
211–216 ↗(On Diff #26731)

We have three files that are managing the state here, which is confusing and hard to follow.v

Three files? After these changes there are only two - search-utils and message-search. Do you mean the context?

For that reason, I wonder if it would make sense to have these effects in the same file, right next to each other.

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.

Make useSearchMessages return a callback

This revision is now accepted and ready to land.May 23 2023, 2:31 AM
ashoat added inline comments.
lib/shared/search-utils.js
189 ↗(On Diff #26849)

We should always be flexible with input parameters

This revision was automatically updated to reflect the committed changes.