Optimise multiple api requests with Promise.all()

Question

I'm trying to optimise multiple made to an 3rd party API (Spotify) in my NestJS API. My code is working but I do not find a way to optimise it.

 @Get('')
 async getAllData(@Req() req: Request) {
    const token = req.cookies.access_token;
    const user = this.userService.getCurrentUserProfile(token);

    const periods = ['short_term', 'medium_term', 'long_term'];
    const types = ['artists', 'tracks'];

    const getUserTopData = async () => {
      const promise = [];
      types.forEach((type) => {
        periods.forEach(async (period) => {
          promise.push(
            this.personalizationService.getUserTopArtistsTracks(
              type as 'artists' | 'tracks',
              {
                time_range: period as
                  | 'short_term'
                  | 'medium_term'
                  | 'long_term',
                limit: 1,
              },
              token,
            ),
          );
        });
      });
      return promise;
    };

    const promises = await getUserTopData();
    const [
      artistsShortTerm,
      artistsMediumTerm,
      artistsLongTerm,
      tracksShortTerm,
      tracksMediumTerm,
      tracksLongTerm,
    ] = promises;

    return Promise.all([
      user,
      artistsShortTerm,
      artistsMediumTerm,
      artistsLongTerm,
      tracksShortTerm,
      tracksMediumTerm,
      tracksLongTerm,
    ])
      .then((values) => {
        const user = values[0];
        const artistsShortTerm = values[1];
        const artistsMediumTerm = values[2];
        const artistsLongTerm = values[3];
        const tracksShortTerm = values[4];
        const tracksMediumTerm = values[5];
        const tracksLongTerm = values[6];

        return {
          user,
          artistsShortTerm,
          artistsMediumTerm,
          artistsLongTerm,
          tracksShortTerm,
          tracksMediumTerm,
          tracksLongTerm,
        };
      })
      .catch((err) => {
        return err;
      });
  }

this.personalizationService.getUserTopArtistsTracks() function which executes the request :

export class PersonalizationService {
  constructor(private httpService: HttpService) {}

  async getUserTopArtistsTracks(
    type: 'artists' | 'tracks',
    query: {
      time_range: 'short_term' | 'medium_term' | 'long_term';
      limit: number;
      offset?: number;
    },
    token: string,
  ): Promise {
    const topArtists = this.httpService
      .get(
        `https://api.spotify.com/v1/me/top/${type}?${qs.stringify(query)}`,
        {
          headers: { Authorization: `Bearer ${token}` },
        },
      )
      .toPromise();

    return (await topArtists).data;
  }
}

A call to my controller on NestJS return a JSON like that :

I think my code is too long and that there is a more shorter way to do it. But I don't see how.

user_5196394 · Answer

No need to create a promise function if you don't need it, you can return a promise for an async function. Don't use async keyword for a sync function.

@Get('')
 async getAllData(@Req() req: Request) {
    const token = req.cookies.access_token;

    const periods = ['short_term', 'medium_term', 'long_term'];
    const types = ['artists', 'tracks'];

    const getUserTopData = () => {
      const promise = [];
      types.forEach((type) => {
        periods.forEach((period) => {
          promise.push(
            this.personalizationService.getUserTopArtistsTracks(
              type as 'artists' | 'tracks',
              {
                time_range: period as
                  | 'short_term'
                  | 'medium_term'
                  | 'long_term',
                limit: 1,
              },
              token,
            ),
          );
        });
      });
      return promise;
    };

    const [
      user,
      artistsShortTerm,
      artistsMediumTerm,
      artistsLongTerm,
      tracksShortTerm,
      tracksMediumTerm,
      tracksLongTerm,
    ] = await Promise.all([
      this.userService.getCurrentUserProfile(token),
      ...getUserTopData()
    ]);

    return {
      user,
      artistsShortTerm,
      artistsMediumTerm,
      artistsLongTerm,
      tracksShortTerm,
      tracksMediumTerm,
      tracksLongTerm,
    };
  }

user_4912329 · Answer

When you use Promise.all, you are already doing asynchronous requests in parallel. Unless spotify let's you get more data with less API requests, you will not find a better solution since you need to query Spotify.

However, the code clarity/quality can be improved:

The controllers shouldn't handle the logic

So redirect your controller to a service >

@Get("")
async getAllData(@Req() req: Request) {
   const token = req.cookies.access_token;
   // even that can be within the function
   const user = this.userService.getCurrentUserProfile(token);

   return await this.userService.getUserTopData(user, token);
}

Then within your service, define the type directly in your constants.

async getUserTopData(user: User, token: string) {
  try {
    const periods: ("short_term" | "medium_term" | "long_term")[] = ["short_term", "medium_term", "long_term"];
    const types: ("artists" | "tracks")[] = ["artists", "tracks"];

    const promises = [];
     types.forEach(type =>
      periods.forEach(period =>
        promises.push(
          this.personalizationService.getUserTopArtistsTracks(type, { timeRange: period, limit: 1 }, token)
        ),
      ),
    );

    promises.push(this.getCurrentUserProfile(token));

    const [
      user,
      artistsShortTerm,
      artistsMediumTerm,
      artistsLongTerm,
      tracksShortTerm,
      tracksMediumTerm,
      tracksLongTerm,
    ] = await Promise.all(promises);

    return {
      user,
      artistsShortTerm,
      artistsMediumTerm,
      artistsLongTerm,
      tracksShortTerm,
      tracksMediumTerm,
      tracksLongTerm,
    };
  } catch (e) {
    // Do something here
  }
}

Handle potentials errors (try, catch, exception handlers,...)

Tomalak_18771 · Answer

const periods = ['short_term', 'medium_term', 'long_term'];
const types = ['artists', 'tracks'];

// line up promises in an array
const getUserTopData = () =>
  types.flatMap((type) =>
    periods.map((period) =>
      this.personalizationService.getUserTopArtistsTracks(/* ... */)
    )
  )
);

// wait for promises (result order will reflect array order)
const results = await Promise.all(getUserTopData());

// give the result list back its structure
return types.reduce((obj, type) => {
  obj[type] = periods.reduce((obj, period) => {
    obj[period] = results.shift();
    return obj;
  }, {});
  return obj;
}, {});

This will produce a data structure like this:

{
  short_term: {artists: '...', tracks: '...'},
  medium_term: {artists: '...', tracks: '...'},
  long_term: {artists: '...', tracks: '...'}
}

Feel free to unpack the results array differently.

Optimise multiple api requests with Promise.all()

3 Answers