Default onError/onCompleted callback about apollo-feature-requests HOT 9 OPEN

@jerelmiller That is partially my fault, I did start talking more generically about "operations" in later posts. But yes, being able to set defaults for mutations would be a fantastic way to introduce this and, at least in our case, would provide a lot of value. Query support would be nice, but much less impactful.

Aha, I did not know about this debate. I definitely fall into the >3.8 crowd in that case--if there is no network request, there is nothing to "complete". 🤷‍♂️ Surprised to hear that it is highly contentious!

from apollo-feature-requests.

@jxdp would you provide some examples of the kinds of logic you'd use in those global onCompleted/onError callbacks? I suspect you're not using window.alert in a production application 🙂. Are you showing toast messages or doing some kind of logging in here? Any more info would be appreciated. Thanks!

from apollo-feature-requests.

FYI, going to transfer this to the feature requests repo. I'll make sure to get our messaging updated on the other repo the make it less confusing. Thanks for pointing that out!

from apollo-feature-requests.

@jxdp would you provide some examples of the kinds of logic you'd use in those global onCompleted/onError callbacks? I suspect you're not using window.alert in a production application 🙂. Are you showing toast messages or doing some kind of logging in here? Any more info would be appreciated. Thanks!

Hi @jerelmiller, thanks for your response and for moving this to the right place!

We'd love to be able to do set a default that includes some basic logic for logging, capture and surfacing an error to the user.

In code, something along these lines:

onError: (e) => {
  console.error(e); // log it
  captureError(e); // send it to our external error monitoring provider
  setGlobalError(e); // surface it to the user; on internal tools this really might just be `window.alert` instead
}

It is extremely rare that we want to do anything more than the above.

from apollo-feature-requests.

@jxdp for global error logging, have you considered using an Error link? It's well suited for this use case: it receives both network errors and GraphQL errors as the request travels back "up" the link chain.

Fwiw, you could also call window.alert from inside the error link if that's satisfactory for an internal tool :) I do see the value of creating a global error handling mechanism within the React render cycle, though I'm curious about a global onCompleted - is there a specific use case there?

I don't have an answer for when we'll be able to prioritize these global callbacks, but the above may be useful in shipping at least partial support for what you're looking for in the meantime.

from apollo-feature-requests.

@alessbell

Using the Error link for logging seems like a great idea, thanks for that. We utilise an Error link for a certain classes of errors (eg errors caused by an API version mismatch) but I wouldn't want to do anything like window.alert here because it's not possible to change this behaviour dynamically (or at least much more complex).

Being able to set a default implementation for onCompleted would allow for things like having successful mutations automatically trigger a toast message, or updating react state (eg a global counter for how many operations the user has performed). There are a couple of ways to get something close to this behaviour, but they're much more work than just setting a default implementation in the ApolloClient constructor.

from apollo-feature-requests.

@jxdp I'm curious if a custom ApolloLink would work for your use case. You might find it more reliable than onCompleted anyways since its a guaranteed network request. onCompleted is a bit spicy these days since there are many different ideas on the "right" behavior for that callback.

Would something like this work for you?

const successLink = new ApolloLink((operation, forward) => {
  return forward(operation).subscribe({
    next: (result) => {
      if (!result.errors) {
        incrementSuccessCount()
        Toast.success('You did it!')
        // etc.
      }
    }
  })
})

If you need to make it a bit more robust because you might be working with @defer or something that returns a multipart response and don't want to trigger the logic until the request is complete, this might be a bit better.

const successLink = new ApolloLink((operation, forward) => {
  let lastResult;

  return forward(operation).subscribe({
    next: (result) => {
      lastResult = result;
    },
    complete: () => {
      if (!lastResult.errors) {
        incrementSuccessCount()
        Toast.success('You did it!')
        // etc.
      }
    }
  })
})

from apollo-feature-requests.

@jerelmiller

In principal something like that could could work, but I can see it quickly becoming a mass of if/else clauses in order to handle the different operation types and provide escape hatches for specific operations (eg we have some mutations fire off in the background that we don't want to handle in the same manner as when a user actively does something).

IMO the main criteria for this feature are:

1. be able to specify the default behaviour for all success/error for queries/mutations
2. be able to override any of the default behaviour for a specific operation

I think the existing onError and onCompleted callbacks are a compelling choice because they are existing APIs that have basically the right semantics (notwithstanding any recent spiciness, which I don't know the details of). What is missing is the ability to specify the default behaviour of those callbacks (or rather, to change the default behaviour, which is currently to do nothing).

You are right that throwing defer into the mix raises some additional questions, but if this feature builds on top of onError and onCompleted then no new ideas need to be introduced--it will just work however those callbacks work.

from apollo-feature-requests.

@jxdp I'm realizing I think I misunderstood some of your initial requests. I see your initial description mentions mutations, which are much less spicy and controversial.

The spiciness I refer to is for the onCompleted callback on useQuery. We fixed an issue in v3.8.0 that stop onCompleted from being called for cache updates. Some devs relied on this this behavior in their apps, so it broke when upgrading. Essentially though we have 2 groups of devs that think the < 3.7 behavior is correct, and others that think the > 3.8 behavior is correct.

I could definitely see an argument for wanting a default callback on mutations, again because these are much less controversial and are much more straightforward. Its the query callbacks that we are much less hesitant to want to add support for. To be totally transparent, we are weighing the future of those query callbacks in future majors as we've seen them misused and misunderstood quite a few times.

Anyways, appreciate the context here! Just wanted to at least give an alternate idea to see what you're looking for and get a better idea of the types of things you're using it for. Thanks!

from apollo-feature-requests.