1

I'm creating a first draft for a node CLI to run docker stacks and containers.

Every call is made using docker commands, with asynchronous calls and results. So we decided to use promises.

Our main problem now is the famous then cascade, especially when we have to pass values and start spinners/messages along the way.

Here is our main ugly method. I'm trying to figure out how this could be refactored to be more... readable!

upgradeServiceImage({env, stack, service}) {
    if (!env || !stack || !service) {
      throw Error(`You must specify env, stack and service to upgrade a service`);
    }

    let payloadSpinner = ora({
      text: `Getting upgrade payload. Please wait...`
    }).start();

    this.getServiceUpgradePayload({env, stack, service})
      .then((response) => {
        payloadSpinner.succeed(`Upgrade payload retrieved successfuly for service ${stack} > ${service}`);

        let upgradeSpinner = ora({
          text: `Upgrading the service. Please wait...`
        }).start();

        this.upgradeImage(response)
          .then((res) => {
            upgradeSpinner.succeed(`Image upgrade request for service "${service}" was made.`);

            this.checkUpgrade({env, stack, service}).then((res) => {
              let finishUpgradeSpinner = ora({
                text: `Finishing upgrade of service by deleting old container. Please wait...`
              }).start();

              this.finishUpgrade(response).then(() => {
                finishUpgradeSpinner.succeed(`Upgrade is now complete!`);
              })
              .catch((err) => {
                finishUpgradeSpinner.fail(`Finishing upgrade failed. Please see in UI. Err: ${err}`);
              })
            })
            .catch((err) => {
              upgradeSpinner.fail(`${err}`);
            })

          })
          .catch((err) => {
            upgradeSpinner.fail(`Image upgrade failed with error: ${err}`);
          });
      })
      .catch((err) => {
        payloadSpinner.fail(err);
      });
  };

It works perfectly but it's barely readable and modular.

I tried some fixes with Promise.all but I got problems to recover my spinners (loading messages) properly and I wasn't able to pass objects from one promise to another (example: response).

Veve
  • 6,643
  • 5
  • 39
  • 58
BlackHoleGalaxy
  • 9,160
  • 17
  • 59
  • 103
  • can you use async/await? – Kos Sep 06 '17 at 11:02
  • 1
    I'm voting to close this question as off-topic because it asking to refactor the code, it would be more appropriate on https://codereview.stackexchange.com/ – Veve Sep 06 '17 at 11:38
  • It would work on codereview.se too, but I'm voting to not close because the question is about a specific problem with well defined boundaries (`then` cascade). – Kos Sep 06 '17 at 11:44
  • 2
    The fact that the question also fits another stack exchange site doesn't automatically make it off-topic for this site. I can't see why it would be off-topic according to [rules as written](https://stackoverflow.com/help/on-topic) – Kos Sep 06 '17 at 11:47

1 Answers1

3

Given code like this:

let payloadSpinner = ora({
  text: 
}).start();

this.getServiceUpgradePayload({env, stack, service})
  .then((response) => {
    payloadSpinner.succeed(`Upgrade payload retrieved successfuly for service ${stack} > ${service}`);
    // start the second task
  })
  .catch((err) => {
    payloadSpinner.fail(err);
  });

you can rewrite it with a helper that manages the spinner for you:

function withSpinner({ task, textOnStart, textOnSuccess, textOnError }) {
  const spinner = ora({
    text: textOnStart,
  })
  return task.then(value => {
    spinner.succeed(textOnSuccess(value));
    return result;
  }, reason => {
    spinner.fail(reason);
    return Promise.reject(reason);
  });
}

upgradeServiceImage({env, stack, service}) {
  if (!env || !stack || !service) {
    throw Error(`You must specify env, stack and service to upgrade a service`);
  }

  withSpinner({
    task: this.getServiceUpgradePayload({env, stack, service}),
    textOnStart: `Getting upgrade payload. Please wait...`,
    textOnSuccess: result => `Upgrade payload retrieved successfuly for service ${stack} > ${service}`,
    textOnError: error => error,
  }).then(result => {
    // start the second task
  })

Then you can re-use the helper to create spinners for next steps.

Note that the promise returned by withSpinner is rejected, so if the first task fails, the second task won't execute.

Here's a working demo:

/* some scaffolding for the purpose of the example */

let spinnerId = 0;

function ora({ text }) {
  let myId = spinnerId++;
  console.log("Spinner", myId, "start", text);
  return {
    succeed(msg) {
      console.log("Spinner", myId, "succeed", msg);
    },
    fail(msg) {
      console.log("Spinner", myId, "fail", msg);
    }
  };
}

function delay(ms, result) {
  return new Promise(resolve => setTimeout(() => resolve(result), ms));
}
function delayFail(ms, reason) {
  return new Promise((resolve, reject) => setTimeout(() => reject(reason), ms));
}

/* scaffolding ends, actual code begins */

function withSpinner({ task, textOnStart, textOnSuccess, textOnError }) {
  const spinner = ora({
    text: textOnStart,
  })
  return task.then(value => {
    spinner.succeed(textOnSuccess(value));
    return value;
  }, reason => {
    spinner.fail(reason);
    return Promise.reject(reason);
  });
}

function upgradeServiceImage() {
  return withSpinner({
    task: delay(500, "THE UPGRADE PAYLOAD"),
    textOnStart: `Getting upgrade payload. Please wait...`,
    textOnSuccess: result => `Upgrade payload retrieved successfuly: ${result}`,
    textOnError: error => error,
  }).then(result => {
    return withSpinner({
      task: delay(800, "upgradeImage"),
      textOnStart: `Upgrading the service. Please wait...`,
      textOnSuccess: result => `Image upgrade request for service was made.`,
      textOnError: error => error,
    });
  }).then(result => {
    return withSpinner({
      task: delayFail(700, "some kind of error"),
      textOnStart: `Checking upgrade`,
      textOnSuccess: result => `Checking upgrade finished`,
      textOnError: error => `CHecking upgrade failed because ${error}`,
    });
  }).then(result => {
    console.log("this won't run anymore because previous step failed");
  }).catch(error => {
    // additionally log the error if you want
    console.error("catch", error);
  });
};

upgradeServiceImage();

Update: This is how the same code would look like using async / await. Only the upgradeServiceImage function has been modified. This approach is more 'flat' and readable. It also makes it straightforward to use previous results in consecutive tasks.

/* some scaffolding for the purpose of the example */

let spinnerId = 0;

function ora({ text }) {
  let myId = spinnerId++;
  console.log("Spinner", myId, "start", text);
  return {
    succeed(msg) {
      console.log("Spinner", myId, "succeed", msg);
    },
    fail(msg) {
      console.log("Spinner", myId, "fail", msg);
    }
  };
}

function delay(ms, result) {
  return new Promise(resolve => setTimeout(() => resolve(result), ms));
}
function delayFail(ms, reason) {
  return new Promise((resolve, reject) => setTimeout(() => reject(reason), ms));
}

/* scaffolding ends, actual code begins */

function withSpinner({ task, textOnStart, textOnSuccess, textOnError }) {
  const spinner = ora({
    text: textOnStart,
  })
  return task.then(value => {
    spinner.succeed(textOnSuccess(value));
    return value;
  }, reason => {
    spinner.fail(reason);
    return Promise.reject(reason);
  });
}

async function upgradeServiceImage() {
  try {
    const upgradeResult = await withSpinner({
      task: delay(500, "THE UPGRADE PAYLOAD"),
      textOnStart: `Getting upgrade payload. Please wait...`,
      textOnSuccess: result => `Upgrade payload retrieved successfuly: ${result}`,
      textOnError: error => error,
    });
    const upgradeImageResult = await withSpinner({
      task: delay(800, "upgradeImage"),
      textOnStart: `Upgrading the service. Please wait...`,
      textOnSuccess: result => `Image upgrade request for service was made.`,
      textOnError: error => error,
    });
    const anotherResult = await withSpinner({
      task: delayFail(700, "some kind of error"),
      textOnStart: `Checking upgrade`,
      textOnSuccess: result => `Checking upgrade finished`,
      textOnError: error => `CHecking upgrade failed because ${error}`,
    });
    console.log("this won't run anymore because previous step failed");
  } catch (error) {
    // additionally log the error if you want
    console.error("catch", error);
  };
};

upgradeServiceImage();
Kos
  • 70,399
  • 25
  • 169
  • 233
  • Looks nice, but I fail to see how the response from `this.getServiceUpgradePayload` is being used in `this.finishUpgrade `. Could you add some comments to the code snippet to make it a bit more clear? – Alex Blex Sep 06 '17 at 12:01
  • There's a few options to use the response from previous task, [neatly documented here](https://stackoverflow.com/questions/28250680/how-do-i-access-previous-promise-results-in-a-then-chain) - does it help? I can add comments later today if that's OK. – Kos Sep 06 '17 at 12:16
  • @Kos i'm also confused by how I can efficiently pass the reponse to subsequent promises methods – BlackHoleGalaxy Sep 06 '17 at 12:46
  • yes that's what I meant. Previously I was asking if you can use async/await? are you targeting Node 8? this is much much simpler using the new syntax. – Kos Sep 06 '17 at 12:52
  • Yes we are indeed targetting node 8.4+ – BlackHoleGalaxy Sep 06 '17 at 12:52
  • 1
    I added a second example. – Kos Sep 06 '17 at 13:39
  • I want to test the method using something like ```const upgradeImageResult = await withSpinner({ task: this.secondMethod(upgradeResult); ...}); ``` in order to retrieve first result as my second task argument but will all await blocks be executed simultaneously or it will wait at each block? I absolutely need the results form previous calls in the later ones. – BlackHoleGalaxy Sep 07 '17 at 00:14
  • 1
    `const foo = await x` waits until the promise `x` resolves and the variable `x` will have the actual value. If `x` rejects, `await` will throw the reject reason as exception – Kos Sep 07 '17 at 05:40