Allow async onCompleted & onError callbacks

[pieterjandebruyne]

Issue Description

Passing async functions is not typed correctly
Could we change the types of onCompleted and onError to also allow for async functions?

Current:

onCompleted?: (data: TData) => void;
onError?: (error: ApolloError) => void;

Expected:

onCompleted?: (data: TData) => void | Promise<void>;
onError?: (error: ApolloError) => void | Promise<void>;

Link to Reproduction

not-applicable

Reproduction Steps

No response

@apollo/client version

3.9.2

[jerelmiller]

Hey @pieterjandebruyne 👋

void should allow async functions just fine. We just don't do anything with the return value, so we don't need to have your function return a promise.

Check out this playground. You can see async functions work with useMutation and useQuery just fine.

Are you seeing something different? If so, could you put together a TS playground, or some other form of reproduction where you're not seeing this work correctly?

[pieterjandebruyne]

Hi @jerelmiller . Thanks for the quick response. Everything working as intended but because of the mistype some ESLint rules are not being respected, in particular TypeScript rule S6544.
https://typescript-eslint.io/rules/no-misused-promises/#checksvoidreturn
Should have included this in my initial message..

It is possible to disable in eslint configs but I see no issues with changing it inside Apollo code (as you no not do anything with the return types anyways) eg. I know a lot of people use sonarcloud for some eslinting on ci/cd and they have the rule enabled by default so it would show up as a bug when passing promises to onCompleted.

[jerelmiller]

@pieterjandebruyne thanks for that helpful info! I'm going to add this as an agenda item for our next team meeting to chat about. I suspect if we adopt this change, we'll probably want to do a more complete sweep of the codebase to apply it to more areas where returned promises would be acceptable. I'd like to discuss with the team though to best understand what direction we'd like to go. Thanks!

[phryneas]

I have a clarifying question here:

As far as I understand it, the purpose of the ESLint plugin is to catch situations where an error thrown inside an async function would not be handled, meaning when there is no .catch called on the resulting promise and it would cause an uncaught promise rejection.

We do call onCompleted and onError, but if you throw an error in your own code in there, we won't catch that - it would result in an uncaught promise rejection.

Isn't the ESLint plugin doing it's job perfectly here?
I feel if we added those Promise<void> return types, we'd indicate to ESLint that we actually handle that resulting promise - cheating your ESLint rule out of an actual positive finding.

[pieterjandebruyne]

An async functions return type will always be Promise regardless of handling errors correctly or not?

const mockedApiCall = async () => {
    return Promise.resolve(true)
}

const asyncFc = async () => {
    try {
        await mockedApiCall()
    } catch (error) {
        console.error('Error caught', error)
    }
}

I made this TS codepen so you can check the types, even if you would catch the errors, the type of the "asyncFc" will always be Promise

Same would be true when using then/catch syntax, if we would pass this "asyncFc" to the onCompleted callback the eslint error would popup because we pass type () => Promise<void> to a property that expects () => void.

[phryneas]

Yes, that's one of the limitations of this ESLint rule - it just looks at types and not info functions.
That's why ESLint is adding a way to specify "safe promises" for their no-floating-promises rule.

The main question would be here: why do you have this rule? What it it's meaning? What should it actually prevent?

My assumption here is "it should prevent you to pass Promises into places where their errors are not being handled" - and if that's the intention, the rule is currently doing the right thing. We certainly do not handle those promise rejections.

If it is in your code base to do something other than that, please tell me - we can't get to the point of making a decision on this without knowing what this rule is actually meant for.

We don't want to break the intended functionality of ESLint on accident. (I'm also pinging @JoshuaKGoldberg here, maybe he can enlighten us a bit on the intention of that rule).

[JoshuaKGoldberg]

👋 thanks for the ping! If I'm reading this right, what is being asked for here is centered around the no-misused-promises rule (so, +1 to #11571 (comment)). To recap, no-misused-promises enforces that if a location is typed as a function returning void, a function that returns a Promise isn't provided to that location. We can think of that as a kind of sibling to no-floating-promises: it's not that a Promise is floating, it's that code is set up in a way that will likely create floating Promises later on.

Here's a reduction of the code in question:

declare function withOnCompleted<Data>(
  data: Data,
  onCompleted: (data: Data) => void
): void;

declare function myOnCompleted(data: string): Promise<void>;

withOnCompleted("...", myOnCompleted);
//                     ~~~~~~~~~~~~~
// Promise returned in function argument where a void return was expected.

If the contents of withOnCompleted call onCompleted(data), they wouldn't know to handle the created Promise.

So, yes, +1 from me on this types request 😄. If an API is meant to take in a function that's allowed to return a Promise, then it'd be better (from the linting perspective) to mention that in the types. That way the lint rules can let folks know when they're passing a Promise-returning function in a place that can't handle it appropriately.

Is that what you're looking for @phryneas?

[phryneas]

So, yes, +1 from me on this types request 😄. If an API is meant to take in a function that's allowed to return a Promise, then it'd be better (from the linting perspective) to mention that in the types.

I think you interpreted this the wrong way round here: we don't expect a function that can return a promise here - we just get a callback, execute it and don't do anything with the return value.
If that function would return an unhandled promise, it would stay unhandled.

That way the lint rules can let folks know when they're passing a Promise-returning function in a place that can't handle it appropriately.

So yeah, we are one of those situations: we do not handle a promise-returning function that would be passed in here appropriately, hence I guess the rule should actually trigger here, and it would be a bad idea for us to circumvent the warning?

[JoshuaKGoldberg]

Oh! Yes you're right and my interpretation of the API was wrong 😄 awesome. no-misused-promises is right to complain then!

[phryneas]

I think on that basis I'd say that we should not be changing our types, as it would imply some kind of security to our users that actually isn't the case.

We also can't just handle promise rejections on these functions in our code base since there's just not a "right thing to do" for us - if that functions throws, it's very unclear how we should go about it and what the result should be.
Should we swallow it? Log it? Have it end up in the error object? None of these would be a good solution for everyone.

I guess if you don't want the ESLint rule to trigger in these situations, you'll need to disable it - or you'd have to write code that won't trigger it.

So in this case, instead of

onCompleted: async () => {
  // code
}

you'd have to write some kind of IIFE

onCompleted: () => {
  (async () => {
    // code
  })().catch(e => {
    // handle errors
  })
}

I know that's kind of a pain, but you have enabled this ESLint rule because you want it to be strict here - if we were to change our types here, it would just be "cheating around the eslint rule", but not actually help you solve the issue that it points at.

=> I'm going to close this issue, since there is nothing on our side that we really can do about this.
You'll have to make the decision on which route to take and how strict you want to be in your code base with your team.

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.