Deceptive Promises behavior

The most part of applications written in JS nowadays uses at least few calls of Promises API, some of them uses es5 syntax, others async/await. But sometimes incomplete understanding of this technology(as in any other) can lead to unpredictable behavior, which can confuse uses, and take hours for you to understand the cause of the problem.

Spending too much time in writing JS code, i've found interesting case with promises: promises have an API which can lead to incorrect interpreting of potential result.

This is mostly related to classic es5 promise realization, but, alas, also affects async/await promises realization.

Lets as an example check the process of saving user:

const handleSave = userData => {
  saveUser(rawUserData)
    .then(user => showNotification(`User ${getUserName(user)} has been created`))
    .catch(err => showNotification(`User was not created because of error`));
};

This code looks easy to read, but not easy to predict potential edge case. While trying to be explicit, we have attached our catch not only for the saveUser request, but also for the onFulfilled block. Thus, if then throws the error (e.g. the getUserName function throws) then the user will be notified that user creation failed with error, even though it was.

Someone might think, that switching the order of the then/catch blocks, so that the catch is attached to the saveUser call directly. This paves the way for another issue.

Using async/await approach will not necessarily help. It is agnostic to using the API correctly, and because of its block scoping it also makes it easier and prettier to write it dangerously as above:

const handleSave = async userData => {
  try {
    const user = await saveUser(userData);
    showNotification(`User ${getUserName(user)} has been created`);
  } catch(error) {
    showNotification(`User was not created because of error`));
  }
};

As you can see, this code has the same issue as above.

To avoid this behavior(when using native Promise API) we need to pass 2 callbacks(error callback, success callback) into then block in correct order, which feels harder to read.

const handleSave = userData => {
  saveUser(userData)
    .then(
      user => showNotifications(`User ${getUserName(user)} has been created`),
      err => showNotifications(`User was not created because of error`));
    );
};

To be clear, this isn't a bad API in itself. But considering the rightful intention of being explicit as a developer, there is a temptation of using a named function for each, rather than one then with the two callbacks. The responsible code is less explicit and readable than dangerous code - it is temptingly dangerous to misuse the API - while feeling more explicit and readable!

The responsible refactor using async/await looks strange. Having to define variables in a higher scope feels like a bad control flow. It feels like we're working against the language:

const handleSave = async userData => {
  try {
    const user = await saveUser(rawUserData)
        .catch(() => showNotifications(`User could not be saved`))

    showNotifications(`User ${displayName(user)} has been created`);
  } catch(error) {
    console.error(`User could not be saved`));
  }
};

Whereas the examples above be dangerous because they could be incorrectly interpret by developers the catch is meant to be attached to the "root" async call - there is also a danger with long chains of thinking the catch is associated with the most recent then.

For example:

const createUserHandler = userData => {
  saveUser(userData)
    .then(sendWelcomeMessage)
    .catch(sendErrorMessage)
};

this looks and reads easier, compared to the responsible:

const createUserHandler = userData => {
  saveUser(userData)
    .then(user =>
      sendWelcomeMessage(user)
        .catch(sendErrorMessage)
    );
};

Lets go further, to see another way how the API can be dangerous: lets add additional logging for if the user can't be created:

const createUserHandler = userData => {
  saveUser(userData)
    .catch(logUserCreationError)
    .then(sendWelcomeEmail)
    .catch(sendErrorMessageByEmail)
};

What we want is to write the issue to our logs if the user save fails, but if sendWelcomeMessage failed, we will need to send error message for user email.

However, because catch block doesn't re-throw or reject, it returns a resolved promise and so the next then block which calls sendWelcomeEmail will be triggered, and because there is no user, it will throw, and we will create a email for a non-existing user.

So, the fix looks ugly the same as for the example above:

const createUserHandler = userData => {
  saveUser(userData)
    .then(
      logIssues,
      user =>
          sendWelcomeEmail(user)
            .catch(sendErrorMessageByEmail)
      );
};

To summarize, we've seen how promise's API for handling errors while seemingly sleek, can be dangerous when developer is moving towards the way of readability.