RFC: urql-react: executeQuery should return a promise with the query result, same as the mutation

[Penguibird]

Summary

It is sometimes more useful to call a query imperatively (e.g. in an event handler) and use its results in the same function. Currently this requires to get the useClient hook and client.query which is less ergonomic. While the useQuery hook returns an executeQuery function, this doesn't return a promise, in contrast to the useMutation hook.

Proposed Solution

The executeQuery return by useQuery would return a promise allowing you to use it as such:

const [{data}, executeQuery] = useMyGeneratedQuery({pause: true});

const handleClick = useCallback(async () => {
  const {data} = await executeQuery();
  // use data here
},[]);

This is a small change, and would mainly be a QoL, but also prevent a lot of mistakes that come from manually using client.query (stale data being the main one).

[kitten]

There's some prior issues and discussions re. this, but I can't find them right now. But the gist of them is that this isn't desireable and won't be implemented (in any of the bindings, for that matter, unless it suits the framework's patterns)

Personally, I've never seen a use-case emerge for what you're proposing, basically 😅 Not to disrespect the request. I know it may feel natural, but it leads to bad patterns and is generally not something that makes sense, imho. It's often used to lump both imperative logic together with non-imperative logic, i.e. reactive UI + reactive after imperative execution, and this leads to a different class of mistake that's more severe than the alternative.

It's always recommended to use client.query(...).toPromise() for imperative queries, first of, as per the above distinction. (On a side note, I can see useMyGeneratedQuery in your example, but TypedDocumentNodes are now generally recommended and the QoL improvement you're probably looking for)

As per the stale state, you're mentioning. It's unexpected for client.query(...).toPromise to lead to stale/unready results, hence it filters them out and doesn't resolve on them: https://github.com/urql-graphql/urql/blob/9ba4130729cc323370afaadf418f66c6691d1317/packages/core/src/utils/streamUtils.ts#L17

On a side-note, I've often also seen this request in conjunction with queries that should really not be queries, hence my hesitation 😅 But I've also seen use-cases where things obviously must/should be queries, but await executeQuery is requested when it isn't needed.

On a side-note, executeQuery returning a promise wouldn't even be semantically correct tbh, and would be the same as calling it + calling client.query(...).toPromise in parallel, most of the time.

[Penguibird]

Thanks, I wasn't aware client.query does the stale filtering, this is very helpful!

As for my use cases:

The first one is a quirky API in which certain queries mutate server state and therefore I want to run the query after a user action, await it, check success/error and show the appropriate modal. I understand this is not how a graphql API should behave, but it is nonetheless one that I have to work with.

The second one is in my opinion more valid: The app concerns golf course reservations. There is an input form with data about a user - name, email, unique number as well as a price for entry. The initial data is fetched from the server, but afterwards the price can be manually edited to allow for whatever discounts. There's a state object with all the user data that gets sent to the server on submit.

When you enter in a user's information into the form, I run a query to the server to check for the prices. I then need to use the response to update the state with the new price.

There is literally no good way to do this without an imperative call.

There are similar use cases for updating state based on query calls. I don't think the interaction is that rare or weird and it would be much nicer with this change.

You are correct that client.query with the generated documents and types works, especially if (as I found out) i don't have to wrap it to check for stale data.

[kitten]

When you enter in a user's information into the form, I run a query to the server to check for the prices. I then need to use the response to update the state with the new price.

that sounds to me like it doesn't require executeQuery to return a Promise, but instaed, sounds to me like you're initializing state. This can be done once using a client.query(...).toPromise call, however, otherwise, useQuery is there to force you to handle query updates.

In other words, it sounds to me like you're trying to have your cake and eat it too 😅 You want useQuery to change, but you also want it to be manually triggered and executeQuery to give you a promise so you can use that data to update state separately.

I'd recommend to think of clear boundaries of when state is preserved or reset, maybe to hoist the query to separate state initialisation, and then to make the manual edits on overlay on top of the queried data.

Anyway, I recognise that this seems like an impossible problem sometimes. But a lot of these problems start when you're trying to mix UI state with your server state, i.e. when you're trying to combine too much state too early.

Just based on a form that's derived from a query and another mutation that uses a different query's result, I don't really see how this needs await executeQuery, basically 😅 Sorry!

[Penguibird]

Understandable. I can see how the state could become a nightmare.

My main reasoning was that afaik useQuery does a bunch of niceties over just client.query(). Stuff like cleanup on component unmount, the stale check (which I wasnt aware is in client.query), making sure the old query gets disposed when variables change. While you can add these yourself, I thought it would be nice to just be able to use executeQuery.

[kitten]

Stuff like cleanup on component unmount

To be clear, it just needs to mount an effect. That's the same as doing client.query(...).subscribe which returns an unsubscribe function.

In other words, useQuery subscribes to updates to the query, so when a new result comes in, it also updates, since we're not just dealing with a Promise-like single deferred result. So, that's not needed when you use client.query(...).toPromise(), since it'll only give you one result.

When you convert the source of OperationResults to a promise, you're basically only getting a single result, and the source is disposed afterwards. Conversely, btw, if there's multiple active subscribers to a single query, they'll all get the same updates, and deduplication should happen behind the scenes.

[kitten]

Closing this as the original question was resolved with an alternative approach. Mainly — and I'm happy to obviously continue discussing the original proposal — we have gotten asked about this in the past.

However, I still believe that adding this API and encouraging this pattern over client.query(...).toPromise leads to weird React code that often isn't as reliable. Encouraging people to explicitly use either or both simultaneously is a more conscious choice.

This also protects us from cases where the original useQuery hook becomes unmounted or the source becomes outdated while a promise was returned from executeQuery. If such a Promise doesn't basically just do client.query(...).toPromise and instead reuses the active source it could stall. In other words, an implementation would be equivalent to this.