Retry API

[igorkamyshev]

This API will provide a declarative way to set up reties for particular Query or Mutation.

Use case

HTTP request could fail by many reasons:

• not a good time for a client — e.g. user could go through underpass and lost their connectivity
• not a good time for a server — e.g. temporary lost connectivity between services on server

Restrictions

Check on particular error

Requests with some errors could be retried, some could not.

The API have to provide a way to distinguish particular error before starting retry.

Dynamic timeout

If the error was caused by server problems, it could be dangerous to retry the request immediately — it could lead to "internal DDoS".

The API have to provide a way to define retry interval dynamically as a Sourced field.

Dynamic number of retries

Same as timeout.

Parameters modification

In some systems, it is important to tell the server that the current request is a retry.

The API have to provide a way to modify Query/Mutation parameters on every retry.

API proposal

Based on the provided use cases and restrictions, I purpose to add new operator retry with the following API:

function retry({
  source: 
    | Query<Params, ..., Error>
    | Mutation<Params, ..., Error>
    | Array<Query<Params, ..., Error> | Mutation<Params, ..., Error>>,
  filter: (error: Error) => boolean,
  timeout: TwoArgsSourcedField<Params, Meta /* retry number, etc. */, number, ExternalTimeoutSource>,
  retries: Sourced<Params, number, ExternalRetriesSource>,
  mapParams?: TwoArgsSourcedField<Params, Meta /* retry number, etc. */, number, ExternalMappingSource>,
})

Usage example

retry({
  source: locationQuery,
  filter: isNetworkError,
  timeout: (params, { retryNumber }) => retryNumber * 1000,
  retries: 10,
})

[igorkamyshev]

In my opinion, we also could provide a set of common presets for timeout — e.g. exponential timeout is pretty common. So, we can provide exponentialTimeout as a part of public API.

[igorkamyshev]

Related to #51

[igorkamyshev]

Should it be in core or in the separate add on?

[AlexandrHoroshih]

I feel a bit weird about this api

What if i apply retry twice to the same query? With different settings in different parts of the code:

// page-a.model.ts
retry({
  source: locationQuery,
  filter: isNetworkError,
  timeout: (params, { retryNumber }) => retryNumber * 1000,
  retries: 10,
})

// page-b.model.ts
retry({
  source: locationQuery,
  filter: isNetworkError,
  timeout: (params, { retryNumber }) => retryNumber * 10,
  retries: 2,
})

Which behavoiur will be active and when?

Looks like it should not be separate logical operator and rather should be part of initial Query configuration, which is a bit less confusing 🤔

[igorkamyshev]

I've thought about it, let me explain a bit.

Let's start with tree-shaking. What if you do not want to apply reties to your queries? It would be a quite tricky to make it tree-shakable, I assume something like this would work, but it has its own problems too 👇

const query = createQuery({
  // ...
  addOns: [
    retry({
      source: locationQuery,
      timeout: (params, { retryNumber }) => retryNumber * 10,
      retries: 2,
    })
  ]
})

• it makes types even more verbose, so I am a bit afraid of giga-sample, so it is a real concern for me
• it requires additional work for users who are going to create custom factories, they have to support add-ons on type-vele, they have to apply it somehow in they factories, so it leads to poor DX in my opinion

Ok, we can make an exception for retries and consider it as an essential part, so we are fallbaking to something like this:

const query = createQuery({
  // ...
  retry: {
    source: locationQuery,
    timeout: (params, { retryNumber }) => retryNumber * 10,
    retries: 2,
  }:
})

Yeah, it's easier to cover with types and we can provide helpers in createHeadlessQuery for custom factories, but what should we do with other possible addons? Shall we put it in the same place? cache? Trigger API?

In my mind, this approach leads API to blowing, but I want to keep it small and composable.

Ecosystem

Farfetched is based on Effetor, let's look on its API 👇

const $attempt = createStore(0)
const newAttempt = createEvent()

sample({
  clock: newAttemp,
  source: $attempt,
  target: attempt => attempt + 1,
  target: $attempt,
})

It uses separate methods to create connection, why we do not worry about duplication? What if we write domething like this 👇

const $attempt = createStore(0)
const newAttempt = createEvent()

sample({
  clock: newAttemp,
  source: $attempt,
  target: attempt => attempt + 1,
  target: $attempt,
})

sample({
  clock: newAttemp,
  source: $attempt,
  target: attempt => attempt + 15,
  target: $attempt,
})

Of course, it would break application logic, but it is okay, because it is explicit error in the code.

So, now let's rewrite future retry with pure Effector's API:

const $attempt = createStore(0)
const newAttempt = createEvent()

sample({
  clock: newAttemp,
  source: $attempt,
  target: attempt => attempt + 1,
  target: $attempt,
})

sample({
  clock: query.finished.failure,
  source: $attempt,
  filter: (attempt, { params, error }) => isNetworkError(error) && attempt < 10,
  fn: (_, { params }) => params,
  target: [newAttempt, query.start]
})

As you can see, it is only couple of samples. Now, it feels weird to rewrite in the inline way, isn't it?

const newAttempt = createEvent()
const $attempt = createStore(0, {
  [newAttempt]: attempt => attempt + 1
})

// ...

Conslusion

I sure, that suggest API (method retry) is not the best one, we have to think about it a lot before implementing. But I insist, that separate methods for additional behaviur is better that new fields in factory config.

---

Sorry for such a long answer 😹 Are you convinced now?

[igorkamyshev]

Oh, I forgot to write the part about composable API's, let me do it now 🤗

Separate methods for modified Query behaviour has one another benefit — composability. Let's imagine, you want to create some helper for your internal Queries (I mean, in your company). With separate methods you can simply create you own method and apply it to any Query 👇

function enhanceExploreQuery(query) {
  retry({ source: query, /* ... */ })
  cache({ source: query, /* ... */ })
}

Created enhancer could be used for any Query — created by built-in factories, custom factories or whatever. I cannot imagine the same for inline configs way without breaking types or DX for custom factories creators 🤔

[AlexandrHoroshih]

These are good arguments, thanks!
Now i understand the design better.

Maybe this operator needs some additional "context-based" filter then? I can imagine a case, where retry rules are different depending on, e.g. which page user is currently on

Something like that:

retry({
  enabled: profilePage.$open,
  source: locationQuery,
  filter: isNetworkError,
  timeout: (params, { retryNumber }) => retryNumber * 10,
  retries: 2,
})

[yumauri]

Does it cover case with lost authentication?

Like
→ when error 401 came
→ queue query
→ make another authenticate query
→ after successful authentication unqueue original query
→ retry it

Or this is out of scope for this API?

[igorkamyshev]

Does it cover case with lost authentication?

Like → when error 401 came → queue query → make another authenticate query → after successful authentication unqueue original query → retry it

Or this is out of scope for this API?

It looks like a case for something like connectQuery 🤔 could you create a separate issue with detailed description of this case, please?

[yumauri]

I would not say this is an issue, more like a use case :) But I can describe it separately nonetheless, sure.

As far as I understand, this API is for unexpected floating errors, like "429 Too Many Requests", whereas lost authentication is an expected error. Am I right?

[igorkamyshev]

Yeah, you are right 🤗

[igorkamyshev]

After some discussion, we have decided to change source to query, and make filter regular sourced field ✅