Node.js - Promise execution order issue

Question

I have an issue with ordering of an async operation executes. Below is my code and it sends a request to a 3rd party API to create invoice items and then another request to create the invoice. My problem is, the invoice is created before the adjustment invoice item is created.

function createInvoiceItems(invoice, customerDetails) {
  return new Promise((resolve, reject) => {
    const invoiceItems = invoice.invoiceItems;
    const invoiceCurrency = invoice.currency;

    const promises = invoiceItems.map((invoiceItem) =>
      new Promise((fulfill, deny) => {
        invoiceItem.currency = invoiceCurrency;

        createInvoiceItem(customerDetails, invoiceItem)
          .then((item) => fulfill(invoiceItem.amount))
          .catch((error) => deny(error));
      })
    );

    return Promise.all(promises)
      .then((lineTotals) => {
        if (lineTotals.length > 0) {
          const invoiceTotal = calculateInvoiceTotal(lineTotals);
          console.log('INVOICE TOTALS ######################', invoiceTotal);
          const customerCommitment = invoice.commitment;

          //Create adjustment invoice item
          if (invoiceTotal < customerCommitment) {
            const adjustmentAmount = (customerCommitment - invoiceTotal);
            console.log('ADJUSTMENT AMOUNT ######################', adjustmentAmount);
            const invoiceItem = {
              currency: invoiceCurrency,
              amount: adjustmentAmount,
              description: 'Adjustment',
            };

            createInvoiceItem(customerDetails, invoiceItem)
              .then((item) => {
                if (typeof item === 'object') {
                  return resolve(customerDetails.stripeId);
                }
              })
              .catch((error) => reject(error));
          }
        }
      })
      .then(() => resolve(customerDetails.stripeId))
      .catch((error) => reject(error));
  });
}

/**
 * Generates customer invoices
 * @param invoices
 */
function generateInvoices(invoices) {
  return new Promise((resolveInvoice, rejectInvoice) => {
    const promises = invoices.map((invoice) =>
      new Promise((resolve, reject) => {
        let invoiceDetails;

        getInvoice(invoice)
          .then((invoiceObject) => {
            invoiceDetails = invoiceObject;
            return getCustomerDetails(invoiceDetails.customerId);
          })
          .then((customerDetails) => createInvoiceItems(invoiceDetails, customerDetails))
          .then((customerId) => createInvoice(customerId))
          .then((thirdPartyInvoice) => {
            invoiceDetails.close = true;

            updateInvoiceFile(invoiceDetails)
              .then(() => resolve(thirdPartyInvoice.id));
          })
          .catch((error) => reject(error));
      })
    );

    Promise.all(promises)
      .then((invoice) => {
        if (invoice.length > 0) {
          return resolveInvoice(invoice);
        }
        return rejectInvoice('Invoice could not be generated');
      })
      .catch((error) => rejectInvoice(error));
  });
}

What is the issue with this code?

Answer 1

This happens because of the following construct:

.then(… => {
    …
    createInvoiceItem(customerDetails, invoiceItem)
    .then(… => {
        resolve(customerDetails.stripeId);
    })
})
.then(() => resolve(…))

Since you don't return anything from the first callback, the promise chain doesn't wait for anything, and calls resolve immediately in the second callback. The resolve being called after createInvoiceItem comes too late.

But your real issue is the prevalent use of the Promise constructor antipattern. You must not use the Promise constructor anywhere - then chains! This simplifies your code a whole lot:

function createInvoiceItems(invoice, customerDetails) {
  const invoiceItems = invoice.invoiceItems;
  const invoiceCurrency = invoice.currency;

  const promises = invoiceItems.map(invoiceItem => {
    invoiceItem.currency = invoiceCurrency;
    return createInvoiceItem(customerDetails, invoiceItem)
      .then(item => invoiceItem.amount);
  });

  return Promise.all(promises)
    .then(lineTotals => {
      if (lineTotals.length > 0) {
        const invoiceTotal = calculateInvoiceTotal(lineTotals);
        console.log('INVOICE TOTALS ######################', invoiceTotal);
        const customerCommitment = invoice.commitment;

        //Create adjustment invoice item
        if (invoiceTotal < customerCommitment) {
          const adjustmentAmount = (customerCommitment - invoiceTotal);
          console.log('ADJUSTMENT AMOUNT ######################', adjustmentAmount);
          const invoiceItem = {
            currency: invoiceCurrency,
            amount: adjustmentAmount,
            description: 'Adjustment',
          };

          return createInvoiceItem(customerDetails, invoiceItem)
            .then(item => {
              if (typeof item === 'object') {
                return customerDetails.stripeId;
              }
              // not sure what should happen otherwise?
            });
        }
      } // else
      return customerDetails.stripeId;
    });
}

function generateInvoices(invoices) {
  const promises = invoices.map(invoice =>
    getInvoice(invoice)
    .then(invoiceObject =>
      getCustomerDetails(invoiceDetails.customerId);
      .then(customerDetails => createInvoiceItems(invoiceDetails, customerDetails))
      .then(customerId => createInvoice(customerId))
      .then(thirdPartyInvoice => {
        invoiceObject.close = true;
        return updateInvoiceFile(invoiceDetails)
        .then(() => thirdPartyInvoice.id);
      })
    )
  );

  return Promise.all(promises)
  .then(invoice => {
    if (invoice.length > 0)
      return invoice;
    else
      throw new Error('Invoice could not be generated');
  });
}