Javascript Promise looping multiple times

Question

I have a Javascript promise that returns a list of items.

findMessages(chatItem: any): Promise<any[]> {
    return new Promise<any[]>((resolve) => {
        let items: any[] = [];
        let promiseArray: Promise<any>[] = [];
        this.storage.keys().then((keys: string[]) => {
            for (let i: number = 0; i < keys.length; i++) {
                if (keys[i] && keys[i].startsWith(this.me.uid + 'message/')) {
                    let promise: Promise<any> = this.storage.get(keys[i])
                    promise.then((data: string) => {
                        let item: any = JSON.parse(data);
                        if ((item && item.memberId1 === chatItem.memberId1 && item.memberId2 === chatItem.memberId2)
                            || (item && item.memberId1 === chatItem.memberId2 && item.memberId2 === chatItem.memberId1)) {
                            items.push(item);
                        }
                    });
                    promiseArray.push(promise);
                }
            }
            Promise.all(promiseArray).then(() => {
                resolve(items);
            });
        });
    });
}

This Promise is called below.

findMessages(chatItem: any): Observable<any[]> {
    return Observable.create((observer) => {
        this.firebaseDataService.findMessages(chatItem).forEach(firebaseItems => {
            this.localDataService.findMessages(chatItem).then((localItems: any[]) => {
                let mergedItems: any[] = this.arrayUnique(firebaseItems.concat(localItems), false);
                mergedItems.sort((a, b) => {
                    return parseFloat(a.negativtimestamp) - parseFloat(b.negativtimestamp);
                });
                if (this.me && mergedItems && mergedItems[0] && this.me.uid === mergedItems[0].memberId2) {
                    this.updateChatWithMessage(chatItem, mergedItems[0], false);
                }
                observer.next(mergedItems);
            });
        });
    });
}

Problem

After the promise call, I expect the following line to only be called once:

let mergedItems: any[] = this.arrayUnique(f....

However, if I place a breakpoint on this line, it is called multiple times when returning from the promise. It gets called the amount of times equal to the number of items returned from the promise (localitems). As if it's looping on localitems.

I have probably structured my code incorrectly, any advise would be appreciated.

Thanks

wouldn't that line be called as many times as there are firebaseItems - because that code is within this "loop" `this.firebaseDataService.findMessages(chatItem).forEach(firebaseItems => {` — Jaromanda X, May 08 '17 at 06:48
In my test example, I only have 2 firebase items, and over 100 local items. It's getting called over 100 times. The firebase promise only gets called once in the `forEach`, but returns a list of items. I would expect the promise of local items to return once with a list of items too. But it is returning multiple times with a list of items. — Richard, May 08 '17 at 06:50
The `firebaseItems` has 2 items, and the `localItems` has 121 items. My breakpoint on the line above is being called 121 times. — Richard, May 08 '17 at 06:53
so how many times does `this.localDataService.findMessages(chatItem)` get run — Jaromanda X, May 08 '17 at 06:54
`this.localDataService.findMessages(chatItem)` only gets called once — Richard, May 08 '17 at 06:56
Avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, May 08 '17 at 06:56
@Richard - I think I'm confused, because you're referring to `firebaseItems` as having two items, but the real question is the length of the array returned by `this.firebaseDataService.findMessages(chatItem)` - otherwise `this.localDataService.findMessages(chatItem)` only being called once makes no sense (the problem is my understanding, not your comments) — Jaromanda X, May 08 '17 at 06:58
`firebaseItems` contains 2 items, but the `this.localDataService.findMessages(chatItem)` is only called once. The reason for this is the `forEach` returns a list of 2 items. That is expected behaviour, I originally also thought a `forEach` should loop for each item returned, but instead it just returns the list of items, which can be looped with a `for` statement if required. Hope this makes sense? — Richard, May 08 '17 at 07:05
Looking at Peter Grainger's answer below, which seems to have resolved my issue, my comment above does not make sense. I need to just test this a bit, and will comment when I understand the issue. Apologies for the confusion. — Richard, May 08 '17 at 07:11
To be honest I am pretty confused. Peter Grainger's has resolved my issue. the line is now not being called multiple times. However, I increased the `firebaseItems` to 20 items, and the `forEach` is only called once still. So his solution seems to have fixed my issue, but I don't really understand why. — Richard, May 08 '17 at 07:20
Hi @Bergi, I am struggling to get my head around the anti-pattern concept. Would you be able to give an example applicable to my example? — Richard, May 08 '17 at 08:16
@Richard Remove the `return new Promise((resolve) => {` wrapper thingy, and just directly **`return`** the `this.storage.keys().then(…)` chain and `Promise.all(promiseArray)` from their respective functions. — Bergi, May 08 '17 at 15:23
Thanks Bergi, yes I see that will improve the code. But I am still faced with the same issue of the function being called multiple times, when I only want it to be called once. — Richard, May 08 '17 at 17:34

score 1 · Answer 1 · answered May 08 '17 at 06:55

As you aren't using the firebase item as an argument in retrieving the promise you can make the call that returns a promise before you run the for each loop. I'm guessing your problem is that as the promise is inside the for each loop it is firing off a load of promises that are being resolved for each firebase item.

findMessages(chatItem: any): Observable<any[]> {
    return Observable.create((observer) => {
            this.localDataService.findMessages(chatItem).then((localItems: any[]) => {
        this.firebaseDataService.findMessages(chatItem).forEach(firebaseItems => {
                let mergedItems: any[] = this.arrayUnique(firebaseItems.concat(localItems), false);
                mergedItems.sort((a, b) => {
                    return parseFloat(a.negativtimestamp) - parseFloat(b.negativtimestamp);
                });
                if (this.me && mergedItems && mergedItems[0] && this.me.uid === mergedItems[0].memberId2) {
                    this.updateChatWithMessage(chatItem, mergedItems[0], false);
                }
                observer.next(mergedItems);
            });
        });
    });
}

Thank you, that does seem to have fixed the issue. It's now not being called multiple times. I just need to test it a bit, and then will mark this as the answer. — Richard, May 08 '17 at 07:08
Unfortunately this answer doesn't work for me. For 2 reasons, it is still being called multiple times, and it means that when a new item is added to the local items, the display is not updated with the new item, because it's not being 'observed' anymore. I think I may need to change the local items, from a `Promise` to an `Observable'. But I'm not sure how this will affect the original multiple calling issue. — Richard, May 08 '17 at 08:01
I think I need to also try understand the deferred antipattern as commented above. — Richard, May 08 '17 at 08:03

Javascript Promise looping multiple times

1 Answers1