This repository has been archived by the owner on Nov 11, 2023. It is now read-only.
stabilize the identity of useGet and its refetch method #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Deploy request for restful-react accepted. Accepted with commit 3f02d9d https://app.netlify.com/sites/restful-react/deploys/608096b392ab900008f13730 |
|
Sorry, I missed the notification, will review this tomorrow! Just as a sidenote, I did saw this on twitter recently: https://epicreact.dev/the-latest-ref-pattern-in-react/ and of course, I thought about this old issue ^^ This could be neat way to solve this! |
fabien0102
reviewed
Apr 21, 2021
fabien0102
reviewed
Apr 21, 2021
fabien0102
reviewed
Apr 21, 2021
| await wait(() => expect(children).toBeCalledTimes(3)); | ||
| expect(children.mock.calls[2][0].loading).toEqual(false); | ||
| expect(children.mock.calls[2][0].data).toEqual({ id: 1 }); | ||
| // A mystery... this is called an extra time |
There was a problem hiding this comment.
Even if this not a big issue, a bit of investigation over there could be nice (at least to understand ^^)
Stabilizing the identity of the `fetchData` function presents some
challenges. The biggest is that `fetchData` needs to have access to
`state` and `setState`, but it needs to have the most up-to-date version
of them in order to work. There are two ways to have done this: the
first is to move the definition of `fetchData` into the function body of
`useGet` itself and make it a closure over the `state` and `setState`
variables - that is the approach taken here. Alternatively, we could
define `fetchData` as a function which takes `state` and `setState` as
parameters, but is closed over a particular combination of props and
context values - this option wasn't explored.
The end result is that the identity of the `refetch` function is stable
given a certain set of props and context, and it is stable over a deep
comparison thereof - so both of the following work fine:
```typescript
const queryParams = { page: 1 };
function MyComponent = () => {
const { refetch } = useGet({ queryParams, lazy: true })`
...
};
```
```typescript
function MyComponent = () => {
const { refetch } = useGet({ { page: 1 }, lazy: true })`
...
};
```
This should resolve contiamo#186 fully (I hope!)
amacleay-cohere
force-pushed
the
stabilize-use-fetch
branch
from
d899887
to
3f02d9d
Compare
fabien0102
approved these changes
Apr 22, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stabilizing the identity of the
fetchDatafunction presents somechallenges. The biggest is that
fetchDataneeds to have access tostateandsetState, but it needs to have the most up-to-date versionof them in order to work. There are two ways to have done this: the
first is to move the definition of
fetchDatainto the function body ofuseGetitself and make it a closure over thestateandsetStatevariables - that is the approach taken here. Alternatively, we could
define
fetchDataas a function which takesstateandsetStateasparameters, but is closed over a particular combination of props and
context values - this option wasn't explored.
The end result is that the identity of the
refetchfunction is stablegiven a certain set of props and context, and it is stable over a deep
comparison thereof - so both of the following work fine:
This should resolve #186 fully (I hope!)
Why
Solves #186
NB
There are 3 tests which suffer from the same symptom: for a reason I haven't completely tracked down, there is an extra re-render dispatched by the hook with identical state: in both instances, it is when the re-render is called, there are 2 returns out of the hook with
loading: true. I don't suppose this will cause any issues with any callers, but I was unable to totally root it out.Note that this uses the same
useDeepCompareCallbackthat is also present in #338 - the implementation in either case is identical and could be rebased out safely depending on merge order.