ref: decouple useApiQuery from useApi #86289
Conversation
the main use-case for useApi is to cancel requests when components unmount; however, for useApiQuery, this feature was disabled thanks to `persistInFlight: true`, because we actually prefer that requests that were started can finish, because the result will wind up in the cache even so, react-query supports query cancellation with an AbortSignal, so if that's a feature we want, we can opt into it. Further, query cancellation is not tied to component unmounting, but can also be used with `queryClient.cancelQueries` imperatively for mutations, it's also not a good idea to make them cancelable, as mutations execute side effects on the server that cannot reliably be canceled once they have been started.
github-actions
bot
added
the
Scope: Frontend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #86289 +/- ##
===========================================
+ Coverage 46.62% 87.90% +41.27%
===========================================
Files 9722 9721 -1
Lines 551687 550699 -988
Branches 21539 21443 -96
===========================================
+ Hits 257242 484066 +226824
+ Misses 294059 66266 -227793
+ Partials 386 367 -19 |
| @@ -127,7 +125,7 @@ export default function useFetchParallelPages<Data>({ | |||
|
|
|||
| const [data, , resp] = await queryClient.fetchQuery({ | |||
| queryKey: getQueryKey({cursor, per_page: perPage}), | |||
| queryFn: fetchDataQuery(api), | |||
| queryFn: fetchDataQuery<Data>, | |||
There was a problem hiding this comment.
fetchDataQuery can now be invoked directly - it doesn’t have to be a function that returns a function. Also, it now defaults to unknown instead of any for data that is returned, but it accepts a type parameter to specify the return type. This helps us not accidentally leaking any around.
| // | ||
| // [0]: https://tanstack.com/query/v4/docs/guides/query-cancellation#default-behavior | ||
| const PERSIST_IN_FLIGHT = true; | ||
| const QUERY_API_CLIENT = new Client(); |
There was a problem hiding this comment.
this is the main change: we are now using a Client singleton for all requests being made by useApiQuery. I think this is fine because we keep it local (unexported), and we don’t do anything with it anymore, like calling .clear().
alternatively, we could just call new Client() directly inside of every queryFn. This would spawn a new client for each request, and it would be garbage collected right afterwards. As far as I can see, a Client is just a class with some properties, so it would be quite cheap to do that.
note that this is also a first step towards a potential queryOptions abstraction, as we’ve now effectively decoupled useApiQuery from any hooks except useQuery.
@JonasBa @evanpurkhiser @ryan953 please let me know what you prefer, I think both approaches would be fine.
| * | ||
| * See also: fetchDataQuery & fetchMutation | ||
| */ | ||
| export function fetchInfiniteQuery<TResponseData>(api: Client) { |
There was a problem hiding this comment.
this wasn’t used outside this file so I just inlined it into useInfiniteApiQuery.
rather than relying on the implementation detail that useApi is used inside useApiQuery
static/app/__mocks__/api.tsx
Outdated
| const errorResponse = Object.assign( | ||
| { | ||
| new RequestError(options.method || 'GET', url, new Error(), { |
There was a problem hiding this comment.
the mock response wasn’t rejecting with a RequestError like the production does; this should narrow the gap
There was a problem hiding this comment.
I will live long enough to see this mock client be removed one day :)
| jest.spyOn(useApi, 'default').mockReturnValue(api); | ||
| jest.spyOn(api, 'requestPromise').mockRejectedValue(new Error()); | ||
| MockApiClient.addMockResponse({ | ||
| url: '/organizations/org-slug/issues/group-id/events/recommended/', | ||
| statusCode: 500, | ||
| }); |
There was a problem hiding this comment.
this doesn’t work anymore, because it depended on the fact that useApiQuery was built with useApi. Spying on implementation details like that isn’t great, so mocking the response is likely better.
| // XXX: our API client mock implementation does not mimick the real | ||
| // implementation, so we need to check for an empty object here. #sad | ||
| const isEmptyObject = err.toString() === '[object Object]'; | ||
| const message = isEmptyObject | ||
| ? t('Error: Unable to load profiles') | ||
| : err.toString(); | ||
| const message = err.toString(); |
There was a problem hiding this comment.
@JonasBa the comment indicated that the check as only there for testing, so I removed it because mocking now does return a RequestError.
But I’ve been thinking if err.toString() is the correct behaviour for production, as it will likely yield a string with RequestError: as prefix. Would we not want the string Error: Unable to load profiles in production as well?
the main use-case for useApi is to cancel requests when components unmount; however, for useApiQuery, this feature was disabled thanks to
persistInFlight: true, because we actually prefer that requests that were started can finish, because the result will wind up in the cacheeven so, react-query supports query cancellation with an AbortSignal, so if that's a feature we want, we can opt into it. Further, query cancellation is not tied to component unmounting, but can also be used with
queryClient.cancelQueriesimperativelyfor mutations, it's also not a good idea to make them cancelable, as mutations execute side effects on the server that cannot reliably be canceled once they have been started.