Details

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	It never makes sense to have an optional parameter before a required one. Please avoid this going forward!
187	Can you give this a better name? Maybe `onResultsReceived`?
211–216	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?

ashoat mentioned this in D7774: [native] Add MessageSearchContent.May 22 2023, 7:22 AM

inka planned changes to this revision.May 23 2023, 1:38 AM

inka added inline comments.May 23 2023, 2:30 AM

lib/shared/search-utils.js
211–216	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.