4

I'm trying to figure out why this bit of code isn't working. I'm looping through a dependency tree (deepObject), and for each one I want to run a function which returns a promise. I would then like for the next set of functions to happen after the resolution of all the promises, but the console logs inside the promise.all are not executing. ES6 is cool too, if you have a better approach, however I would also really like to know why this code is not working.

updated to add .catch (this had no effect)

for (let i = 0; i < keys.length; i++) {
    promisesArray.push(promiseFunction(deepObject[keys[i]]));
}
Promise.all(promisesArray).then(resolved => {
    console.log('dep promises resolved');
    console.log(`resolved: ${JSON.stringify(resolved)}`);
}).catch(err => console.error(new Error(err));

// promiseFunction is recursive
promiseFunction(obj) {
    return new Promise((resolve, reject) => {

        const keys = Object.keys(deepObj);
        for (let j = 0; j < keys.length; j++) {
            if (Object.keys(deepObj[keys[j]]).length) {
                // adding return statement to following line breaks it
                // that was already recommended, see threads
                promiseFunction(deepObj[keys[j]]);
            } else {
                const dep = new Package(keys[j]);
                console.log(`skan dep: ${keys[j]}`);
                dep.fetch().then(() => {
                    return anotherFunction();
                }).then(() => {
                    return Promise.all([
                        promise1(),
                        promise2(),
                        promise3(),
                    ]).then(resolved => {
                        if (resolved[0].propertyINeed) {
                            resolve(true);
                        }
                        resolve(false);
                    });
                }).catch(err => {
                    console.error(new Error(err));
                    process.exit(1);
                });
            }
        }
    });

I know this conversation - has been discussed - on here before

In the second link above the accepted answer suggests:

In case you are asynchronously filling the array, you should get a promise for that array, and use .then(Promise.all.bind(Promise)). If you don't know when you stop adding promises, this is impossible anyway as they might never all be resolved at all.

However I'm not using async to fill the array. Do I need this? What am I missing here?

update

Since I have error logging now in both the .then() as well as the .catch() it seems like something is going wrong unrelated to what happens inside of it.

Adding the return statement before promiseFunction(deepObj[keys[j]]); broke the recursion. I went from iterating over 173 objects to 68. Adding the catch logged no additional results. Code above is updated to share more of the recursive fn. When I run it, it seems to execute all the promises, but I have no way of knowing that. I'm most concerned with knowing 1. that the promise array for promise.all contains all the objects it should and 2. catching the moment when all of those promises for all the objects inside the recursed object are resolved.

Also, for the record, each one of these functions that return promises are all necessarily async. I have gone back through all of this multiple times in an attempt to simplify and remove any unnecessary promises. The task is just complex. There are a series of steps that must execute, they are async, and they are chained because they must be resolved in a specific order.

Community
  • 1
  • 1
Kraken
  • 5,043
  • 3
  • 25
  • 46
  • 4
    try console logging the reject function also to see if something is failing – Semi-Friends Jun 11 '18 at 09:33
  • 3
    "promiseFunction omitted for brevity " —Don't do that. You don't need to provide the real thing, but you do need to provide a [mcve]. I've guessed at what code could go in the missing places and I can't reproduce the problem: http://jsbin.com/fesudezuja/1/edit?js,console – Quentin Jun 11 '18 at 09:34
  • 3
    Try `Promise.all(promisesArray).then(...).catch(err => console.error('Failed', err))`. – dfsq Jun 11 '18 at 09:36
  • Does a trivial promiseFunction work, e.g. just returning Promise.resolve ()? – Terry Lennox Jun 11 '18 at 09:38
  • seconded was @dfsq says. You've most likely thrown an error but you're not logging it.and you don't know why your code "doesn't work" – Adam Jenkins Jun 11 '18 at 09:38
  • ...or one of the promises is not resolving... – Jonas Wilms Jun 11 '18 at 09:41
  • Agreed. Even assuming `promiseFunction` returns a promise, it's likely that you have an exception that you're not logging. Use always `catch` at the end. With `Promise.all` one failure in one promise is enough, even if you have hundreds that succeeds. – ZER0 Jun 11 '18 at 09:43
  • 1
    In `promiseFunction`, the first block of the if statement should `return` the result of the second `promiseFunction` - otherwhise it will not resolve to anything! – Fabio Lolli Jun 11 '18 at 09:45
  • I added the catch statement, but it never logs anything. – Kraken Jun 11 '18 at 09:50
  • I've tried to address all comments in the latest update. – Kraken Jun 11 '18 at 10:52
  • Is it `Promise.all(depArray)` or `Promise.all(promisesArray)`? Please post complete code including the declaration of all the variables, and the `keys` as well. – Bergi Jun 11 '18 at 11:03
  • 2
    Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Jun 11 '18 at 11:04
  • This is a bit messy because you use a few different patterns/ideas mixed into one - it's nothing too crazy, but a bit messy... If I were you, I'd start with replicating the same functionality from the outermost logic with some "mock functions" that return resolved promises (ex: `return Promise.resolve(true)` is helpful for debugging and stubbing functionality) and make it work - then progressively go down layers by substituting actual implementations to these mock promises, until you find the point where the app breaks. You do not NEED to rewrite it, but that's my suggested solution :) – Fabio Lolli Jun 11 '18 at 11:12
  • The behavior that occurs inside the recursion works. It is executed at the top level and I'm now ultimately refactoring it to add it within the recursion. I looked into the promise constructor antipattern, and I can assure you that I have removed all unnecessary promises. There is just a lot of async work happening here and many of the steps depend on one another. – Kraken Jun 11 '18 at 11:35

2 Answers2

4
if (Object.keys(obj[keys[j]]).length) {
    promiseFunction(obj[keys[j]]);
} else {
    // and some more async hell that all resolves correctly

If Object.keys(obj[keys[j]]).length is true then you never call resolve or reject so the promise never resolves.

(Note that calling promiseFunction recursively creates a new promise which is never put into Promise.all).

Since some of your promises don't resolve, Promise.all won't either.


You probably need something more along the lines of:

var promises = [];
processObject(obj);
Promise.all(promises).then(etc etc);

function processObject(object) {
    for ( loop; over; object ) {
        if (condition) {
             processObject(object[something]);
        } else {
             promises.push(new Promise( (res, rej) => {
                 // so something async
             });
        }
    }
}
Quentin
  • 914,110
  • 126
  • 1,211
  • 1,335
  • I've tried to address all comments in the latest update. – Kraken Jun 11 '18 at 10:52
  • The recursion does indeed contain resolve/reject statements – Kraken Jun 11 '18 at 10:53
  • 1
    Not the right ones. I refer you back to the part where I said: (Note that calling promiseFunction recursively creates a new promise which is never put into Promise.all). – Quentin Jun 11 '18 at 10:58
  • Why are you using `new Promise` to begin with? – Benjamin Gruenbaum Jun 11 '18 at 11:47
  • @BenjaminGruenbaum this is being consumed by a parent function. Quentin: promiseFunction returns a promise, the resolution of this promise depends on the successful resolution of the inner promise.all. Therefore, I think they're being accounted for. – Kraken Jun 11 '18 at 12:47
  • 1
    @Kraken — "promiseFunction returns a promise" — That creates Promise A. If you hit the `if` statement then it calls itself recursively. That creates Promise B. If that hits the `else` then Promise B is resolved. Promise A is not resolved. – Quentin Jun 11 '18 at 12:52
  • okay, I see what you're saying, but that doesn't really get to the original question, does it? I'm trying to figure out why I'm not getting any output from either the .then or the .catch with the currently existing promises. We're going to add testing to make sure we're properly iterating all of the objects. But this Q is really about why my promise.all is not logging anything – Kraken Jun 11 '18 at 12:54
  • @Kraken — I already explained that. Since you never resolve Promise A, and Promise A is one of the ones passed to Promise.all, you never resolve all the promises passed to Promise.all, so Promise.all never resolves. – Quentin Jun 11 '18 at 12:58
  • @Quentin, I see. So this would not be handled by the catch? – Kraken Jun 11 '18 at 13:13
  • Correct, Promise A isn't rejected either, so the catch won't trigger. – Quentin Jun 11 '18 at 13:13
0

Let's have a look at your promiseFunction...

for (let j = 0; j < keys.length; j++) {
    if (Object.keys(obj[keys[j]]).length) {
        //if you get in here, promiseFunction returns undefined, and it never gets resolved
        promiseFunction(obj[keys[j]]);
    } else {
        const dep = new Package(keys[j]);
        console.log(`skan dep: ${keys[j]}`);
        // and some more async hell that all resolves correctly
        //but even if it resolves correctly, you still have the if...
    }
}

To do the trick, just return your promiseFunction result in the if statement:

for (let j = 0; j < keys.length; j++) {
    if (Object.keys(obj[keys[j]]).length) {
        //if you get in here, you're saying this promise will depend on how this second promiseFunction will resolve
        return promiseFunction(obj[keys[j]]);
    } else {
        //omitted for brevity...
        resolve('your value');
    }
}

Now when you go into the if statement, you're basically telling the promise to solve based on whether that second promiseFunction has solved correctly - you're delegating the resolution to another Promise.

I never heard anybody give it this name, but you can think of this as Promise recursion :)

Fabio Lolli
  • 859
  • 7
  • 23