Promise chaining - Code Review

1. If the URL includes the right query parameters, then it will send a POST request to the backend

2. Otherwise, it will throw you to the Generic error page.

<template>
  <div class="ek-container">
    <component
      :is="WidgetComponents[getWidgetComponents()]"
      :customer="currentCustomer"
      :agreement="currentAgreement"
      :originalAgreement="originalAgreement"
      :product="currentProduct"
      :originalProduct="originalProduct"
      :insuredObject="currentInsuredObject"
      :originalInsuredObject="originalInsuredObject"
      :coverage="currentCoverage"
      :originalCoverage="originalCoverage"
      :isLoadingDetails="isLoadingDetails"
      :errorDetails="errorDetailsComputed"
    />
  </div>
</template>
<script>
function getWidgetComponents() {
  const productCode = currentProduct.value?.code;

  if (
    !showErrorPage.value &&
    hasValidURLparams.value &&
    hasValidProduct.value
  ) {
    if (productCode === ProductCodes.PREGNANCY) {
      return "PregnancyWidget";
    }
  }
  return "GenericErrorPage";
}

... other codes
</script>

Code Summary

1. Template: A div container with a dynamic component (<component>) that conditionally renders components based on getWidgetComponents(). It passes several props (customer, agreement, product, etc.) to the rendered component.

2. Dynamic Component Resolution: The getWidgetComponents() function determines the component type based on:
• Validity of URL parameters (hasValidURLparams).
• Product validity (hasValidProduct).
• Whether productCode matches ProductCodes.PREGNANCY.

3. Conditional Rendering: If conditions are met and the product code is "Pregnancy," the PregnancyWidget component is shown; otherwise, it defaults to GenericErrorPage.

Positive Points

• Modular and Reusable: Using dynamic components (:is) allows flexibility to display different widgets based on business logic, making it highly reusable.

• Error Handling: The fallback GenericErrorPage provides a safe default if parameters are invalid or the product doesn’t match.

• Parameterization: Passing specific props (customer, agreement, etc.) enables flexibility for the child component to use relevant data.

Negative Points

• Tight Coupling of Conditions: The component heavily relies on a specific product code for conditional rendering. Adding new conditions (like other product types) may require more code duplication or refactoring.

• Limited Extensibility: If new product types require new widgets, the getWidgetComponents() function could become cluttered and harder to maintain.

• Code Organization: getWidgetComponents is embedded in the script without proper separation, making it less modular. Extracting it to a helper or computed property would improve readability and testability.

Recommendations

onMounted(async () => {
  ciaStore.showInlineError = false;

  const areRequestSame = areSameURLParamsRequest(
    ciaStore.lastValidURLParams,
    props.widgetProps ?? ({} as WidgetProps)
  );

  const isSystemError = ErrorCode.SYSTEM_ERROR.includes(
    ciaStore.lastErrorCode
  );

  const shouldSendRequest =
    hasValidURLparams?.value &&
    hasValidProduct.value &&
    (!areRequestSame || isSystemError);

  if (shouldSendRequest) {
    ciaStore.$reset();

    ciaStore.lastValidURLParams =
      props.widgetProps ?? ({} as WidgetProps);

    await createChangeOfferRequest().catch(
      (errors: Error | any) => {
        setTimeout(() => {
          const errorDetails = getErrorMessageDetails(
            errors?.message ?? "",
            ciaStore.commonDrupalText as CommonDrupalText
          );
          errorDetailsRef.value = errorDetails;
          ciaStore.isFullscreenLoading =
            errorDetails.errorKey === ErrorKey.SystemErrors;
        }, 300);
      }
    );
  }
});

1. Why do we need .then if you’re already using await. The await keyword pauses execution until the createChangeOfferRequest() promise resolves or rejects, allowing you to handle the result or error directly without chaining .then or .catch. However, if you want to catch errors when using await, you can use a try...catch block instead of .catch.

2. We are using setTimeout. We can not trust setTimeout as it can lead to a lot of side effects. JavaScript is single-threaded. Meaning, it can only calculate one thing at a time. But now imagine you have a setTimeout that is suppose to fire after 10000ms or 10s. So now JS have to keep track of the time passed. But in between that 10s the user might do some interaction with your page. Now JavaScript have to react to them as well. Here is a very nice blog article about why we should be careful with setTimeout.

1. The use of multiple nested functions and promise chains makes the code difficult to read and maintain.

2. Error handling is scattered across different layers, making it challenging to manage and debug effectively.

3. The reliance on setTimeout for error handling introduces potential timing issues and unnecessary delays.

async function createChangeOfferRequest() {
  if (
    props.widgetProps?.agreementId &&
    props.widgetProps?.customerNo
  ) {
    const payload = {
      policyNumber: props.widgetProps?.agreementId,
      customerId: props.widgetProps?.customerNo,
      customerType: "privateCustomer",
      changeStartDate: moment
        .utc()
        .format("YYYY-MM-DDTHH:mm:ss.SSS[Z]")
    };
    await ciaStore.createChangeOffer(payload);
  }
}

1. First of all, why do we need this second layer of function?

export const changeOfferActions = {
  async createChangeOffer(
    changeOfferRequest: ChangeOfferRequest
  ): Promise<void> {
    const state = useChangeOfferStore();
    try {
      state.isFullscreenLoading = true;
      const changeOfferResponse = await createChangeOffer(
        changeOfferRequest
      );
      updateEntities(
        state.originalEntities,
        changeOfferResponse
      );
      updateEntities(state.entities, changeOfferResponse);
      state.offerId =
        changeOfferResponse.agreement?.offerNo ?? '';
      state.customerId =
        changeOfferRequest.customerId ?? '';
    } catch (error) {
      logInDev('Error creating change offer:', error);
      throw error;
    }
    state.isFullscreenLoading = false;
  },
  .... 
};

1. Why do we need this third layer?

2. Issue: state.isFullscreenLoading is set to false only after the try...catch block, meaning it will remain true if an error occurs.

3. Encapsulated State Updates: Keeping state mutation logic in dedicated state functions, like updateEntities, isolates side effects and improves testability.

export async function createChangeOffer(
  changeOfferRequest: ChangeOfferRequest
): Promise<Offer> {
  return new Promise(async (resolve, reject) => {
    await POST("/api/changePolicy/createChangeOffer", {
      headers: getAPIHeaders(),
      body: changeOfferRequest
    })
      .then((responseFromServer) => {
        if (responseFromServer.error) {
          reject(responseFromServer.error);
        } else {
          resolve(responseFromServer.data as Offer);
        }
      })
      .catch((error) => {
        if (
          error instanceof TypeError &&
          error.message === "Failed to fetch"
        ) {
          reject(new Error(ErrorCode.SYSTEM_ERROR));
        } else {
          reject(error);
        }
      });
  });
}

1. First of all, given the goal of the component, why do we need this?

2. Issue: Wrapping async within new Promise is redundant and unnecessary, as async functions inherently return promises. This leads to more complex code than needed.

3. Issue: The current function handles only a TypeError with a specific message ("Failed to fetch"). It’s best to broaden the error handling to catch other network-related issues (e.g., 5xx server errors, bad request errors). Solution: Check the response status and structure the error handling to cover more cases.

4. Issue: Using .then() and .catch() within an async function reduces readability, as await with try...catch is generally clearer for handling asynchronous code.

import createClient, { Middleware } from "openapi-fetch";

const apiMiddleware: Middleware = {
  async onRequest({ request }) {
    try {
      return request;
    } catch (error) {
      throw error;
    }
  },
  async onResponse({ response }) {
    return response;
  }
};


export const { POST, GET } = client;

Refactored Version

const { mutate } = useMutation({
  mutationFn: createChangeOffer,
  onSuccess: (response) => {
    const offerResponse = response.data as Offer;
    updateEntities(
      ciaStore.originalEntities,
      offerResponse
    );
    updateEntities(ciaStore.entities, offerResponse);
    ciaStore.customerId =
      props.widgetProps?.customerNo ?? "";
    ciaStore.offerId =
      offerResponse?.agreement?.offerNo ?? "";
    ciaStore.requestState = REQUEST_STATE.SUCCESS;
  },
  onError: (error: CustomError) => {
    const errorDetails = getErrorMessageDetails(
      error?.messageCode as ErrorCode,
      ciaStore.commonDrupalText as CommonDrupalText
    );

    errorDetailsRef.value = errorDetails;

    ciaStore.isFullscreenLoading =
      errorDetails.errorKey === ErrorKey.SystemErrors;
    ciaStore.requestState = REQUEST_STATE.ERROR;
  }
});

onMounted(asycn () => {
	// ... other checks
	
	if(valid) {
	const payload = {
      policyNumber: props.widgetProps?.agreementId,
      customerId: props.widgetProps?.customerNo,
      customerType: "privateCustomer",
      changeStartDate: moment
        .utc()
        .format("YYYY-MM-DDTHH:mm:ss.SSS[Z]") 
  };
        
  mutate(payload);
	}
})

export const createChangeOffer = async (
  changeOfferRequest: ChangeOfferRequest
) => {
  return await POST("/api/changePolicy/createChangeOffer", {
    headers: getAPIHeaders(),
    body: changeOfferRequest
  });
};

Advantages

1. Readability: Instead of a 4/5 layer of function chaining, now it just calls one function.

2. With the Tanstack query, I can handle side effects directly without keeping too many variable references.

• Unnecessary promise chaining and complex layered structure in the original code

• Potential side effects from using setTimeout in error handling

• Redundant wrapping of async functions within new Promise

• Limited error handling, focusing only on specific TypeError

• Introduced Tanstack vue query for improved state management and side effect handling

• Simplified API call structure, reducing function layers

• Enhanced readability by eliminating multiple layers of function chaining

• Improved side effect management without relying on numerous variable references

Another version without third party dependencies

import { defineStore } from 'pinia';
import axios from 'axios';

export const useProductStore = defineStore('product', {
  state: () => ({
    products: [],
    isLoading: false,
    error: null,
  }),
  actions: {
    async fetchProducts() {
      this.isLoading = true;
      this.error = null;
      try {
        const response = await axios.get('/api/products');
        this.products = response.data;
      } catch (err) {
        this.error = err.message;
      } finally {
        this.isLoading = false;
      }
    },
  },
});

 onMounted(() => {
    fetchProducts();
 });

Summary

• The Vue component used excessive promise chaining and complex layered structure, making the code difficult to read and maintain

• Unnecessary use of setTimeout for error handling introduced potential timing issues

• Multiple nested functions and scattered error handling made debugging challenging

• Introduced Tanstack Vue Query to improve state management and simplify API calls

• Reduced function layers and eliminated multiple layers of function chaining, enhancing readability

• Improved side effect management without relying on numerous variable references

• Suggested using Pinia actions for direct API calls without third-party dependencies

• Tanstack Query preferred for its built-in state utilities, handling loading, pending, and error states automatically