I'm trying to create a recursive function using Promises, but can't quite seem to get it right. I have working code without using promises, but it uses counters and global variables etc. and doesn't feel quite right, so I'm attempting a rewrite and also creating a module for reuse.

Essentially, the function is supposed to be getting a user from Active Directory and then recursively finding any direct reports and their direct reports and so on.

I've played with lots of versions of the function, this is the current one:

function loadReports(personEmail, list) {
    return new Promise((resolve, reject) => {
        getAccessTokenPromise()
            .then(access_token => {
                list.push(personEmail);
                return makeRequest(personEmail, access_token);
            }).then(result => {
                if (result.value.length > 0) {
                    Promise.all(result.value.map(person => {
                        loadReports(person.userPrincipalName, list);
                    })).then(resolve());
                } else {
                    resolve();
                }
            })
            .catch(e => reject(e));
    });
}

The getAccessTokenPromise function performs a request for an access token and returns a promise for that. The makeRequest function again just makes an https request for the user and their reports and returns a json object with the results as a Promise.

Any thoughts greatly received. Many thanks. D.

I'm trying to create a recursive function using Promises, but can't quite seem to get it right. I have working code without using promises, but it uses counters and global variables etc. and doesn't feel quite right, so I'm attempting a rewrite and also creating a module for reuse.

Essentially, the function is supposed to be getting a user from Active Directory and then recursively finding any direct reports and their direct reports and so on.

I've played with lots of versions of the function, this is the current one:

function loadReports(personEmail, list) {
    return new Promise((resolve, reject) => {
        getAccessTokenPromise()
            .then(access_token => {
                list.push(personEmail);
                return makeRequest(personEmail, access_token);
            }).then(result => {
                if (result.value.length > 0) {
                    Promise.all(result.value.map(person => {
                        loadReports(person.userPrincipalName, list);
                    })).then(resolve());
                } else {
                    resolve();
                }
            })
            .catch(e => reject(e));
    });
}

The getAccessTokenPromise function performs a request for an access token and returns a promise for that. The makeRequest function again just makes an https request for the user and their reports and returns a json object with the results as a Promise.

Any thoughts greatly received. Many thanks. D.

• javascript
• recursion
• promise
• ecmascript-6
• es6-promise

• 1 "but it uses counters and global variables etc" --- now you see how impure functions and free variables are evil. First reimplement it so that it did not rely on variables from the outer scopes, then promisify it. – zerkms Commented Sep 7, 2016 at 5:56

1 Answer 1

To make recursion work with promises, you generally want to chain all the recursive promises to their callers. To do that, you MUST return any promises from your .then() handlers so that they are chained to the originals. This also tends to eliminate your promise anti-pattern of wrapping an existing promise with a manually created promise which is fraught with problems. Here's one way of doing that:

function loadReports(personEmail, list) {
    return getAccessTokenPromise().then(access_token => {
        list.push(personEmail);
        return makeRequest(personEmail, access_token);
    }).then(result => {
        if (result.value.length > 0) {
            return Promise.all(result.value.map(person => {
                return loadReports(person.userPrincipalName, list);
            }));
        }
    });
}

Changes made:

1. Add return before getAccessTokenPromise() so we're returning the initial promise. This also lets us eliminate the new Promise() and all the manual reject and resolve that was involved in the anti-pattern.

2. Add return before the recursive loadReports(). This has to be done to allow the .map() to collect that promise before it passed it to Promise.all().

3. Add return before the Promise.all() so it is chained to the original promise chain.

You will have to make sure that you can never get any sort of circularity in the database data (an error in the DB that creates a circular list of reports). A reports to B, B reports to C, C reports to A because if you did have this, then this code would go on forever and never plete (probably eventually exhausting some system resource).

If this were my code, I might create a Set of all people visited in the database as I go and refuse to call loadReports() on any person who we'd already visited before. This would make it safe from circularity. You would probably also want to log() if you saw that condition because it would probably be a database error/corruption.