How can I avoid nesting promises in this case

Question

How can I make this snippet meet eslint rules, or could I make it more readable/elegant with no nesting promise?

class Sync {
  io = IO.getInstance();

  constructor(token) {
    this.token = token;
  }
  
  sendMessage = (
    data,
    retryTimeout = 0,
  ) => {
    const response = this.io.request(requestConfig); // should return a Promise
    const timeout = retryTimeout + 3000;
    // want to retry when AJAX error
    return response
      .catch(() => new Promise((resolve, reject) => {
        if (this.token === data.token) {
          // eslint warning: Avoid nesting promises.eslint(promise/no-nesting)
          setTimeout(() => this.sendMessage(data, timeout).then(resolve, reject), timeout);
        } else {
          reject(new Error('Failed to send message due to network offline'));
        }
      }));
  }
}

class Sync {
  async sendMessage(data, retryTimeout = 0) {
    const timeout = retryTimeout + 3000;
    try {
      await this.io.request(requestConfig); // should return a Promise
    } catch(e) {
      if (this.token === data.token) {
        await new Promise(resolve => {
          setTimeout(resolve, timeout);
        });
        return this.sendMessage(data, timeout);
      } else{
        throw new Error('Failed to send message due to network offline');
      }
    }
  }

I tried to change this to async function, is this all right now? or how can I make it properly?

Thanks.

score 0 · Accepted Answer · answered Mar 24 '20 at 19:24

0

Using .then(resolve, reject) is the Promise constructor antipattern. Chain it instead:

class Sync {
  sendMessage(data, retryTimeout = 0) {
    const response = this.io.request(requestConfig); // should return a Promise
    const timeout = retryTimeout + 3000;
    // want to retry when AJAX error
    return response.catch(() => {
      if (this.token === data.token) {
        return new Promise(resolve => {
          setTimeout(resolve, timeout);
        }).then(() => this.sendMessage(data, timeout));
      } else {
        throw new Error('Failed to send message due to network offline');
      }
    });
  }

Even better if you refactor the new Promise(…) with the setTimeout into a delay helper function.

answered Mar 24 '20 at 19:24

Bergi

630,263
148
957
1,375

Thx~ Wrap `setTimeout` in a promise is indeed better, but it still has _Avoid nesting promises_ warning, since you call `.then` in `.catch`. – YJ Cai Mar 25 '20 at 08:15
You cannot avoid nesting the `then` call inside the `if` block inside the `catch` callback. It is a totally fine pattern as long as you `return` the inner promise (which you do), so you should ignore the warning here. – Bergi Mar 25 '20 at 14:21
(Alternatively, rewrite the code to `async`/`await`, it probably doesn't complain there) – Bergi Mar 25 '20 at 14:22
I tried to change this to `async` function above, is this all right now? or how can I make it properly? Thanks. – YJ Cai Mar 25 '20 at 16:42
You'll want to `return await this.io.request(requestConfig);`, but apart from that it looks good – Bergi Mar 25 '20 at 16:44
oh，I forgot this. Thank u very much. – YJ Cai Mar 25 '20 at 18:23
> Redundant use of `await` on a return value.eslint(no-return-await) – YJ Cai Mar 26 '20 at 02:35
Uh, [`return await` is fine inside a `try` block](https://stackoverflow.com/a/42750371/1048572), and eslint should know that. – Bergi Mar 26 '20 at 09:15

How can I avoid nesting promises in this case

1 Answers1