-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update useLazyQuery
with planned breaking changes
#12367
base: release-4.0
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 5dcb2df The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
commit: |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
5f45a8c
to
651926c
Compare
options?.fetchPolicy ?? | ||
client.defaultOptions.watchQuery?.fetchPolicy ?? | ||
"cache-first", | ||
fetchPolicy: "standby", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to change the default value for notifyOnNetworkStatusChange
to true
in 4.0. I'm going to reserve this for a separate PR and do this in core so that this change is consistent across the client.
const previousData = resultRef.current?.data; | ||
|
||
if (previousData && !equal(previousData, result.data)) { | ||
// eslint-disable-next-line react-compiler/react-compiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure why this eslint rule complains. I'm also writing resultRef.current
below, but that one is just fine. Any ideas on what I'm doing unsafe here to trigger this lint rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe because you're not necessarily inside an uSES
/useEffect
here and updateResult
could e.g. be called during render.
I bet it will stop complaining once you pass in previousDataRef
as an argument.
} | ||
} | ||
|
||
function handleError(error: unknown) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually this will be moved out of here. We want to update ObservableQuery
in 4.0 so that it never terminates on errors triggered by the link chain. Our React integration has sort of papered over this with the introduction of resubscribeAfterError
to make it seem like the observable continues, but this is an internal API and not meant for end-use.
Once that is in place, we should be able to clean this up quite a bit, if not remove this entirely.
src/react/hooks/useLazyQuery.ts
Outdated
); | ||
|
||
// TODO: Determine if this is still needed. | ||
if (!hasOwnProperty.call(error, "graphQLErrors")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added for legacy reasons, but this should disappear at some point.
|
||
const previousResult = resultRef.current; | ||
if (!previousResult || !equal(error, previousResult.error)) { | ||
updateResult( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be moved to core in a future PR.
useLazyQuery
useLazyQuery
with planned breaking changes
}); | ||
|
||
// TODO: Should we delete this? This is covered by the first test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking yes, but figured I'd ask first, especially since this might make this diff even more difficult to see.
called: false, | ||
loading: false, | ||
networkStatus: NetworkStatus.ready, | ||
previousData: undefined, | ||
variables: { id: 1 }, | ||
variables: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value should be undefined
instead of an empty object since this the variables
type is TVariables | undefined
. This will be addressed in a separate PR so that useQuery
can benefit as well.
variables: {}, | ||
}); | ||
} | ||
expect(result).toEqualApolloQueryResult({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the major changes in this PR. The return type of the execute function is now ApolloQueryResult
rather than QueryResult
(or LazyQueryResult
as this PR adds) which means many of the properties returned on this type are now removed. We used to add everything (refetch
, fetchMore
, etc.) to the resolved value, but this felt excessive. This forces you to use those from the hook instead.
|
||
setTimeout(() => execute()); | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another big change in this PR is that loading states are no longer rendered when calling execute for the first time. Instead you must set notifyOnNetworkStatusChange
in order to receive updates to ANY loading state.
NOTE: A separate PR will be opened in core to change the default of notifyOnNetworkStatusChange
to true
which means this will likely be added back, but wanted to point this out as this better aligns the core API and this hook behavior.
}); | ||
|
||
it("changing queries", async () => { | ||
it("applies changed query to next refetch after execute", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very similar to the "changing queries" test with the difference that we call refetch
instead of the execute function which checks to make sure the query is applied when rerendering this component rather than the next time execute
is called.
// TODO: Determine if this hook makes sense for polling or if that should be | ||
// reserved for useQuery. At the very least, we need to figure out if you can | ||
// start polling a query before it has been executed | ||
it.skip("should allow for the query to start with polling", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drawing attention to this. Lenz and I already discussed this, but wanted to revisit this conversation after we've had a chance to sit on this a bit longer.
I'm still personally torn on how polling should work with useLazyQuery
, mostly due to the dev experience either direction. The declarative API is nice (pollInterval
), but is it confusing that it won't take effect until after you've called the execute function? The imperative API is nice for that reason as its more explicit when it will begin polling, and we can throw an error if you try and use it too early, but it does feel more in-the-way.
I'm also torn about allowing both of them. Its always been unclear to me whether the pollInterval
value or the startPolling
function have higher priority. For example, what happens if you stopPolling()
, then change the pollInterval
value on a future render? Should it start polling because its non-zero, or should that be reserved for startPolling
?
Would love to toss around some more ideas. Regardless, we should probably address both of these together since any decision here might affect useQuery
as well.
Co-authored-by: Lenz Weber-Tronic <[email protected]>
Co-authored-by: Lenz Weber-Tronic <[email protected]>
adcb8e4
to
861e23c
Compare
Closes #5912
Closes #7484
Closes #9317
Closes #10787
Closes #12100
Partially addresses #12272
Adds the planned breaking changes to
useLazyQuery
planned for 4.0. A summary of the changes described in the changesets:useLazyQuery
no longer supports SSR environments and will now throw. If you need to run a query in an SSR environment, useuseQuery
instead.The execute function returned from
useLazyQuery
now only supports thecontext
andvariables
options. This means that passing options supported by the hook no longer override the hook value.To change options, rerender the component with new options. These options will take effect with the next query execution.
The result resolved from the promise returned from the exeucte function in
useLazyQuery
is now anApolloQueryResult
type and no longer includes all the fields returned from theuseLazyQuery
hook tuple.If you need access to the additional properties such as
called
,refetch
, etc. not included inApolloQueryResult
, read them from the hook instead.useLazyQuery
will no longer rerender with the loading state when calling the execute function the first time unless thenotifyOnNetworkStatusChange
option is set totrue
.If you prefer the behavior from 3.x, rerender the component with
notifyOnNetworkStatusChange
set tofalse
after the execute function iscalled the first time.
The
reobserve
option is no longer available in the result returned fromuseLazyQuery
. This was considered an internal API and should not be used directly.useLazyQuery
no longer supports calling the execute function in render and will now throw. If you need to execute the query immediately, consider usinguseQuery
instead.The
defaultOptions
andinitialFetchPolicy
options are no longer supported withuseLazyQuery
.If you use
defaultOptions
, pass those options directly to the hook instead. If you useinitialFetchPolicy
, usefetchPolicy
instead.useLazyQuery
no longer supportsvariables
in the hook options and therefore no longer performs variable merging. The execute function must now be called withvariables
instead.This change means the execute function returned from
useLazyQuery
is more type-safe. The execute function will require you to pass avariables
option if the query type includes required variables.useLazyQuery
will now only execute the query when the execute function is called. PreviouslyuseLazyQuery
would behave likeuseQuery
after the first call to the execute function which means changes to options might perform network requests.You can now safely rerender
useLazyQuery
with new options which will now take effect for the next query.The promise returned when calling the execute function from
useLazyQuery
will now reject when using anerrorPolicy
ofnone
when GraphQL errors are returned from the result.