0

I have been trying to understand the right way to use a for loop for an array of promises. In my code, my array has 3 elements, but it only has data the first time through the loop.

  private populateRequest(connection: Connection, myArray: any[], containerId: string): Promise<void> {
    // tslint:disable-next-line:no-shadowed-variable
    return new Promise(async (resolve, reject) => {
      const promises: Array<Promise<SomeType>> = [];
      let status = '';

      // tslint:disable-next-line:prefer-for-of
      for (let i = 0; i < myArray.length; i++) {
        const data = await this.getResolvedPromise(myArray[i])
        .then(response => response)
        .catch(err => console.log(err));
        if (this.flags.hasOwnProperty('prop1')) {
          status = 'Active';
        } else if (this.flags.hasOwnProperty('prop2')) {
          status = 'Inactive';
        } else if (data[0]['WidgetProp1'] === 'Active') {
          status = 'Inactive';
        } else if (data[0]['WidgetProp1'] === 'Inactive') {
          status = 'Active';
        }

        const myMetaObj = {
          prop3: data[0]['WidgetProp3'],
          status
        };
        const myMetaMember = {
          ContainerId: containerId,
          ContentEntityId: data[0]['WidgetId'],
          Body: data[0]['WidgetBody'],
          Metadata: myMetaObj
        };

        promises.push(myMetaMember);
      }

      Promise.all(promises)
        .then(() => resolve())
        .catch(err => reject(err));
    });
  }

  private getResolvedPromise(target: Promise<any>): Promise<any> {
    // tslint:disable-next-line:no-any
    return new Promise((resolve, reject) => {
    // tslint:disable-next-line:no-any
    Promise.all([target])
     .then(() => resolve(target))
     .catch(err => reject(err));
    });
  }

The push works as intended the first time, but not subsequently.

I understand this is because of async code and calls not finishing, but am not sure why my Promise.all() doesn't work correctly.

Halps?

trebleCode
  • 2,134
  • 19
  • 34
  • There are a bunch of things wrong here, but as I attempt to rewrite this, I need to know what is in the `triggers` array? And, what is in the `myArray` array? What type of data is in there? And, what are you trying to do with `getResolvedPromise()` on the data you take from that array? – jfriend00 Sep 04 '20 at 23:09
  • triggers was supposed to be myArray, and it contains three resolved promises. They all have about 5 properties that I need to use in the rest of the code. I end up sending this data to a SaaS API. I only have get resolved promises because from trying to understand promises I thought I needed a function that could return promise objects. Since I already have the three I need can I scrap get resolved promises and just access the objects by some property like anything else? – trebleCode Sep 04 '20 at 23:14
  • 1
    Also, `getResolvePromise()` does not appear to do anything useful. It calls `Promise.all([target])` and then returns a promise that resolves to `target`. That doesn't appear to be accomplishing anything useful. What did you think that was accomplishing. So, you had target and now you have a promise that resolves to target. Even if `target` was a promise, the caller of `getResolvePromise()` will still have to use `await` or `.then()` to get the result out of that call. – jfriend00 Sep 04 '20 at 23:14
  • Back up and describe what the input to your function is and what output you want and I can probably show you a rewrite that will do that. Right now, it's confusing what data is coming in and what data you want out? – jfriend00 Sep 04 '20 at 23:15
  • You only get resolved values out of promises with `.then()` or `await` or for a group of promises with `Promise.all([...]).then(...)` or `await Promise.all([...])`. Still not sure what you're trying to accomplish yet to know how to suggest a rewrite. FYI, a benefit of TypeScript is supposed to be that you declare actual types so declaring `myArray: any[]` defeats that purpose and does not help me understand what that is supposed to be. – jfriend00 Sep 04 '20 at 23:16
  • Got it. The input is some data from a query against a cloud app in a loop to get some config information I need. Then I am manipulating that data to send back to the app. More or less switching things on and off conditionally. is a specific type the CLI I'm using against the app I have to send back to make the config metadata change – trebleCode Sep 04 '20 at 23:21
  • Is `myArray` an array of promises? Or, an array of something else? If it's an array of promises, what does each promise resolve to? – jfriend00 Sep 04 '20 at 23:22
  • Yes, an array of promises. They all resolve to [object Promise] all with the same properties. – trebleCode Sep 04 '20 at 23:24
  • 1
    And, what is `populateRequest()` supposed to return or do? – jfriend00 Sep 04 '20 at 23:25
  • [Never pass an `async function` as the executor to `new Promise`](https://stackoverflow.com/q/43036229/1048572)! – Bergi Sep 04 '20 at 23:26
  • Thanks @Bergi. `populateRequest()` is supposed to put together the information I need before I send it back to the app for the changes to register – trebleCode Sep 04 '20 at 23:28
  • @trebleCode jfriend is onto something. There's no part in your code where `populateRequest` actually does anything asynchronous. Instead of passing `myArray` as an array of promises, you should pass an array of the resolved values and make `populateRequest` synchronous. Can you please show us where you are calling `populateRequest(…)` and how you construct the promises for the `myArray` parameter? – Bergi Sep 04 '20 at 23:30

2 Answers2

2

There are a couple things wrong here.

First off, you aren't pushing promises into your promises variable. You're pushing myMetaMember variables into that array.

Second, if you're already serializing your asynchronous requests with await, you don't need to use Promise.all() at all.

Third, you're using several anti-patterns by wrapping existing promises in additional layers of manually created promises when you do not need to do that.

Fourth, you generally don't want to mix await and .then() and .catch(). To catch errors from await, use try/catch. There is no need for .then() because await already gets the value for you.

Fifth, getResolvePromise() does not appear to do anything useful. It calls Promise.all([target]) and then returns a promise that resolves to target. That doesn't appear to be accomplishing anything useful. What did you think that was accomplishing.

Since you've now clarified that myArray is an array of promises that each result to an object, here's my best interpretation of what you're trying to do:

private populateRequest(connection: Connection, myArray: any[], containerId: string): Promise<Array<{ContainerId: string; ContentEntityId: unknown; Body: unknown; Metadata: {prop3: unknown; status: string}}>> {
      return Promise.all(myArray).then(results => {
          return results.map(data => {
              let status = '';
              if (this.flags.hasOwnProperty('prop1')) {
                status = 'Active';
              } else if (this.flags.hasOwnProperty('prop2')) {
                status = 'Inactive';
              } else if (data[0]['WidgetProp1'] === 'Active') {
                status = 'Inactive';
              } else if (data[0]['WidgetProp1'] === 'Inactive') {
                status = 'Active';
              }

              const myMetaObj = {
                prop3: data[0]['WidgetProp3'],
                status
              };
              const myMetaMember = {
                ContainerId: containerId,
                ContentEntityId: data[0]['WidgetId'],
                Body: data[0]['WidgetBody'],
                Metadata: myMetaObj
              };
              return myMetaMember;
          });
      });
  }

To get the results from myArray (which you said was an array or promises that each resolve to an object, you would use Promise.all(myArray). That will return a single promise that resolves to an array of results which you can then use .then() to get the array of results. You can then iterate that array to build your new objects based on their data.

P.S. If myArray is really an array of promises, then you shouldn't be declaring it as myArray: any[]. That defeats part of the reason for TypeScript in that it doesn't teach the reader (me) what it is and it doesn't let TypeScript enforce that it's being passed the right thing.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375
jfriend00
  • 683,504
  • 96
  • 985
  • 979
0

i cant really make sense of your example. but what i see is that you are using await which syncronously resolves the promise from getResolvedPromise. You are also using .then in conjunction with await which doesn't make sense as it does the same thing, but just in another style as 'await'. So either you do:

const data = await promise;
// data is resolved here

or

promise.then(
  data => // data is resolved here
)

If you have to iterate over a structure with a for loop instead of using .map i would suggest to do something like this:

async someFunction(): Promise<any[]> { 
  promises: Promise<any>[] = []
  for(const value of list) {
    promises.push(new Promise((resolve, reject) => {
      const data = await something
      // do something with data
      const fancyObject = {}
      resolve(fancyObject);

  }));
  return Promise.all(promises);
}

You see that the logic which is depending on the data object only runs after the promise is resolved via the await operator. So this logic needs to be wrapped in a Promise itself.

Promise.all in it self returns a Promise so you dont need to wrap it in another Promise and resolves to an array containing all your fancyObjects. Beware also that await syntax needs a try catch block in order to catch errors happening while the promise resolves.

I hope i understood you correctly and you can make something of my example.

leonat
  • 127
  • 8
  • Yes this makes sense, thank you very much @leonat . Wrapping my head around it has been fun – trebleCode Sep 04 '20 at 23:23
  • Since we're in an active engagement with the OP to understand what the actual data and function here is supposed to do, it would be better to wait until that becomes clear rather than offer some commentary on part of the issue. We're also trying to get the OP to write a clear question so we can then write a clear answer (which makes StackOverflow a much more useful place). – jfriend00 Sep 04 '20 at 23:24
  • I think the issue is I need to understand await vs .then() better, and that I have a function that isn't at all necessary. – trebleCode Sep 04 '20 at 23:27
  • your comments were not present when i wrote my answer. this is my first engagement with stackoverflow answering so i still have to figure out how the answering process works. claryfing the question before answering makes sense, i will do that next time. Still i think that the whole confusing came out of a misunderstanding how promises work. Thats why i provided a barebone template on how to work with promises in for loops. – leonat Sep 04 '20 at 23:29
  • No problem. I see you were just trying to help (which is good). – jfriend00 Sep 04 '20 at 23:33
  • You are correct @leonat, it was a definitive misunderstanding of promises on my part. Your comment still helped. – trebleCode Sep 04 '20 at 23:33