Implement immediate effect and refactor #250
Conversation
kitten
force-pushed
the
feat/use-query-initial-effect
branch
from
1ca0d9f
to
8bf1dd5
Compare
Currently we manually rely on request.key in useQuery to ensure that we don't trigger a new effect, when it's not necessary, which is error-prone. Instead this hook uses useMemo and checks request.key against the last request it generated. If the key hasn't changed it returns the previous reference. This ensures reference equality, which removes any check of request.key in useQuery
We want to execute an effect immediately in render on initial mount for suspense / SSR. This is a very simple hook that does this by abstracting effects in three phases: - Initial mount (render) - After mount - Render So on initial mount it will execute the effect immediately. Then it will prevent the first run of useEffect since the effect has already been called, then the effect will run as usual.
|
Have made a little seperate branch where I'm testing some implementations for Query and made my suggested changes. Will PR against this branch |
kitten
force-pushed
the
feat/use-query-initial-effect
branch
from
8bf1dd5
to
47a7d62
Compare
kitten
force-pushed
the
feat/use-query-initial-effect
branch
from
47a7d62
to
176fd4b
Compare
|
@JoviDeCroock I've got another PR here that just drops compatibility with older React versions and reuses hooks, if you mean the |
|
That fully removes the need for it yes, allright. I think we're almost there then, just alter the snapshots and we're finished 🎉 your drive on this project is quite inspiring to say the least ^^ We'll also need to find a way to efficiently unsubscribe |
|
@JoviDeCroock Cheers! We're working on fleshing out the core library so we can move on to write more exchanges soon! The unsubscription in useImmediateEffect and the main hooks should be more solid now. Just waiting for a couple of reviews from other core contributors so this can go in 🙌 After this SSR shouldn't be too hard to get done |
|
@andyrichardson I'll merge this for now, but let's have a retroactive review at some point 👍 |
| let { query, variables } = queryGql; | ||
|
|
||
| const { result, rerender } = renderHook( | ||
| ({ query, variables }) => useRequest(query, variables), |
There was a problem hiding this comment.
I think we might be unintentionally shadowing query and variables here. Typically I'll define what's in initialProps to be different from my test data to ensure that all calls to rerender are using the data passed in initialProps or rerender, i.e.:
const { result, rerender } = renderHook(
({ q, v }) => useRequest(q, v),
{ initialProps: { q: query, v: variables }
);
Functionally it doesn't matter b/c query (inside renderHook) -> initialProps.query -> query (from queryGql), and we use let bindings to update the references to variables below, so this is more of a readability suggestion to make it clear where things are coming from 🤗
| /** Creates a request from a query and variables but preserves reference equality if the key isn't changing */ | ||
| export const useRequest = ( | ||
| query: string | DocumentNode, | ||
| variables?: any |
There was a problem hiding this comment.
How would you feel about using a generic here so we can pass it through from useQuery / useSubscription? We could also just use object like we do createRequest to make things easy. Happy to make a quick PR for either if you think it's worthwhile.
Fix #245
Initial stuff that will become necessary for #218
This implements two hooks:
useRequestto simplify the creation of requests while keeping a stable reference, so we don't have to check forrequest.keyuseImmediateEffectto simplify running an effect immediately outside ofuseEffectfor SSR/suspense, which then otherwise behaves like a normaluseEffect.