add doResolvedSearch actions which returns resolved search results #258

2020-01-05T13:02:08+01:00

akinwale commented

2020-01-05 13:02:08 +01:00

(Migrated from github.com)

No description provided.

neb-b (Migrated from github.com) reviewed 2020-01-07 21:35:19 +01:00

neb-b (Migrated from github.com) left a comment

Could this just use the current doSearch and add the flag/change the action types based on some flag passed into the function?

Could this just use the current `doSearch` and add the flag/change the action types based on some flag passed into the function?

akinwale commented

2020-01-08 15:38:34 +01:00

(Migrated from github.com)

I separated this since doSearch already had its own resolve logic, and I didn't want to make the entire thing more complicated than necessary.

I separated this since `doSearch` already had its own resolve logic, and I didn't want to make the entire thing more complicated than necessary.

kauffj (Migrated from github.com) reviewed 2020-01-09 16:33:13 +01:00

src/redux/actions/search.js

					
				@ -162,0 +168,4 @@

				  options: {

				    related_to?: string,

				  } = {}

				) => (dispatch: Dispatch, getState: GetState) => {

kauffj (Migrated from github.com) commented

2020-01-09 16:32:57 +01:00

Do we really have to do this with an entirely parallel chain of calls, as well as a new ResolvedSearchResult type? Why can't this be an additional parameter to doSearch, and when pre-resolved results are returned from the search server, they're stored directly in the claims reducer?

(I realize that part of this is that resolve=1 on lighthouse doesn't actually return the resolved claim, which makes me question the design of this entirely, or at a minimum the name of that parameter)

Do we really have to do this with an entirely parallel chain of calls, as well as a new `ResolvedSearchResult` type? Why can't this be an additional parameter to `doSearch`, and when pre-resolved results are returned from the search server, they're stored directly in the claims reducer? (I realize that part of this is that `resolve=1` on lighthouse doesn't actually return the resolved claim, which makes me question the design of this entirely, or at a minimum the name of that parameter)

akinwale (Migrated from github.com) reviewed 2020-01-09 16:56:34 +01:00

src/redux/actions/search.js

					
				@ -162,0 +168,4 @@

				  options: {

				    related_to?: string,

				  } = {}

				) => (dispatch: Dispatch, getState: GetState) => {

akinwale (Migrated from github.com) commented

2020-01-09 16:56:33 +01:00

The reasoning behind this approach was to have a type with the minimum number of fields required to display a search result item, since I was aiming for speed and better performance (one request vs multiple requests to handle a search). The performance improvement on mobile is positive.

I created a new method instead of using the existing doSearch because I was trying to differentiate the return types (since it ultimately doesn't return a claim, but a subset of values for a particular claim).

@tiger5226 could probably rename the resolve parameter to something else to make it more clear.

The reasoning behind this approach was to have a type with the minimum number of fields required to display a search result item, since I was aiming for speed and better performance (one request vs multiple requests to handle a search). The performance improvement on mobile is positive. I created a new method instead of using the existing `doSearch` because I was trying to differentiate the return types (since it ultimately doesn't return a claim, but a subset of values for a particular claim). @tiger5226 could probably rename the `resolve` parameter to something else to make it more clear.

tiger5226 (Migrated from github.com) reviewed 2020-01-11 05:46:11 +01:00

src/redux/actions/search.js

					
				@ -162,0 +168,4 @@

				  options: {

				    related_to?: string,

				  } = {}

				) => (dispatch: Dispatch, getState: GetState) => {

tiger5226 (Migrated from github.com) commented

2020-01-11 05:46:11 +01:00

I can change it to whatever we want it to be.

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-redux#258