Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use requests PR #159

Merged
merged 20 commits into from
Dec 17, 2019
Merged

Use requests PR #159

merged 20 commits into from
Dec 17, 2019

Conversation

rctbusk
Copy link
Contributor

@rctbusk rctbusk commented Dec 16, 2019

Description

useRequests is a react hook that takes in an array of query configs and returns two values. The first is an object containing values isFinished and isPending. isPending is true as long as any of the network calls made by useRequests is still in progress. isFinished is false as long all requests have not finished. The other value returned is a refresh function. This function when called forces all requests to be recalled no matter the response or status of the previous call.

Upgrade

This is meant to be a replacement to the v2 mapPropsToConfig functionality. Redux Query React did not have a good way of making batch requests in v3.

Architecture

This PR mostly copies both the current useRequest and v2 connect-request to make a hook for an array of query configs.

Feature Request

#158

@rctbusk
Copy link
Contributor Author

rctbusk commented Dec 16, 2019

@mili-confluent, do you have any thoughts on this?

Copy link
Contributor

@Myztiq Myztiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is amazing! Should help with some important use-cases.

);

requestQueryConfigs.forEach(dispatchRequestToRedux);
cancelKeys.forEach(queryKey => dispatchCancelToRedux(queryKey));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a little concern here. Because I also wrote my own useRequests for my company. One edge case is if we first mount a component which has requests [A, B, C] and then we immediately unmount this component so that [A,B,C] will be cancelled (Assume [A,B,C] are all pending before unmounting). If you re-mount that component (for example, reopen that page), it will not request any [A,B,C] as they are already got cancelled. I am not sure whether this code will solve this problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, I saw in useRequest we did some transform on queryConfig,

{retry: true}

https://github.com/amplitude/redux-query/blob/master/packages/redux-query-react/src/hooks/use-request.js#L36-L44

To solve this problem, maybe we also need to do this here?

Copy link
Contributor Author

@rctbusk rctbusk Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I am currently setting {retry: true} in the use-memoized-query-configs.js. So the retry logic is there. This should be moved out and passed into the memoize function. It still has the {retry: true}, but the memoize should be agnostic to this, like how the current use-request.js works. I will change this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's okay, as long as we add {retry: true}

Copy link
Contributor

@mili-confluent mili-confluent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version in someplace is 3.3.0-alpha.7 and someplace is 3.3.0-alpha.6, is that okay? Otherwise, LGTM, thank you.

@rctbusk rctbusk merged commit 2782873 into master Dec 17, 2019
@rctbusk rctbusk deleted the useRequests branch December 17, 2019 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants