6

I have a async recursive function which returns promise if there is more work to do or returns result array otherwise. In case where no recursion is involved it correctly return the array but when recursion is there the array is undefined. The code is

function foo(filepath) {

  var resultArr = [];

  function doo(file) {
        return asyncOperation(file).then(resp => {
            resultArr.push(resp.data);
            if (resp.pages) {
                var pages = resp.pages.split(',');
                pages.forEach(page => {
                    return doo(page);
                });
            } else {
                return resultArr;
            }
        });
    }

    return doo(filepath);
}

And the way this is called

foo(abcfile).then(function(result){
    console.log(result);
});

If I pass abcfile which has no resp.pages, I get result array, but there are resp.pages, then the result array is undefined.

2
  • What are you hoping to do with the pages data? You aren't returning anything in the if (resp.pages) block and the return used in the function passed to forEach doesn't do anything Commented Nov 23, 2016 at 23:50
  • 1
    When resp.pages is truth, you return nothing, which equals return undefined. Commented Nov 24, 2016 at 1:42

4 Answers 4

3

I think you're just missing a returned promise within the if (resp.pages) block

if (resp.pages) {
    return Promise.all(resp.pages.split(',').map(page => doo(page)))
        .then(pagesArr => {
            return resultArr.concat(...pagesArr)
        })
}

I'm thinking there may be an issue with scoping resultArr outside the doo function so maybe try this

function foo(filepath) {
    function doo(file) {
        return asyncOperation(file).then(resp => {
            const resultArr = [ resp.data ]
            if (resp.pages) {
                return Promise.all(resp.pages.split(',').map(page => doo(page)))
                    .then(pagesArr => resultArr.concat(...pagesArr))
            } else {
                return resultArr
            }
        })
    }

    return doo(filePath)
}

To explain the use of the spread-operator, look at it this way...

Say you have three pages for a file top; page1, page2 and page3 and each of those resolves with a couple of sub-pages each, the pagesArr would look like

[
  ['page1', 'page1a', 'page1b'],
  ['page2', 'page2a', 'page2b'],
  ['page3', 'page3a', 'page3b']
]

and resultArr so far looks like

['top']

If you use concat without the spread operator, you'd end up with

[
  "top",
  [
    "page1",
    "page1a",
    "page1b"
  ],
  [
    "page2",
    "page2a",
    "page2b"
  ],
  [
    "page3",
    "page3a",
    "page3b"
  ]
]

But with the spread, you get

[
  "top",
  "page1",
  "page1a",
  "page1b",
  "page2",
  "page2a",
  "page2b",
  "page3",
  "page3a",
  "page3b"
]
Sign up to request clarification or add additional context in comments.

7 Comments

@Phil I think ya scope of the array might be issue along with promises
do i need to use the spread operator ..what does it do? @Phil
@ams I've added an explanation to my answer. If this doesn't work for you, could you provide an example of the desired output format?
Thanks that explains ..I get an error with ... operator ..SyntaxError: Unexpected token ...I have node 4.4.7 @Phil
@Phil nice analysis of the problem – and nice solution too. Parallel processing of the sub-pages is definitely a good way to go as long as 1. there are no circular references (unless you have some elegant solution for that, too! I can't think of one ...) and 2. pages do not have a significantly large number of sub-pages. If 5 pages, each had 5 pages, that each had 5 pages, you'd be up to hundreds of simultaneous requests. Might be good to offer/suggest a queue that can be limited/throttled.
|
1

To verify this works, I'll make a fake dataset, and a fakeAsyncOperation which reads data from the dataset asynchronously. To model your data closely, each query from the fake dataset returns a response with data and pages fields.

let fake = new Map([
  ['root',    {data: 'root',    pages: ['a', 'b', 'c', 'd']}],
  ['a',       {data: 'a',       pages: ['a/a', 'a/a']}],
  ['a/a',     {data: 'a/a',     pages: []}],
  ['a/b',     {data: 'a/b',     pages: ['a/b/a']}],
  ['a/b/a',   {data: 'a/b/a',   pages: []}],
  ['b',       {data: 'b',       pages: ['b/a']}],
  ['b/a',     {data: 'b/a',     pages: ['b/a/a']}],
  ['b/a/a',   {data: 'b/a/a',   pages: ['b/a/a/a']}],
  ['b/a/a/a', {data: 'b/a/a/a', pages: []}],
  ['c',       {data: 'c',       pages: ['c/a', 'c/b', 'c/c', 'c/d']}],
  ['c/a',     {data: 'c/a',     pages: []}],
  ['c/b',     {data: 'c/b',     pages: []}],
  ['c/c',     {data: 'c/c',     pages: []}],
  ['c/d',     {data: 'c/d',     pages: []}],
  ['d',       {data: 'd',       pages: []}]
]);

let fakeAsyncOperation = (page) => {
  return new Promise(resolve => {
    setTimeout(resolve, 100, fake.get(page))
  })
}

Next we have your foo function. I've renamed doo to enqueue because it works like a queue. It has two parameters: acc for keeping track of the accumulated data, and xs (destructured) which is the items in the queue.

I've used the new async/await syntax that makes it particularly nice for dealing with this. We don't have to manually construct any Promises or deal with any manual .then chaining.

I made liberal use of the spread syntax in the recursive call because I its readability, but you could easily replace these for concat calls acc.concat([data]) and xs.concat(pages) if you like that more. – this is functional programming, so just pick an immutable operation you like and use that.

Lastly, unlike other answers that use Promise.all this will process each page in series. If a page were to have 50 subpages, Promise.all would attempt to make 50 requests in parallel and that may be undesired. Converting the program from parallel to serial is not necessarily straightforward, so that is the reason for providing this answer.

function foo (page) {
  async function enqueue (acc, [x,...xs]) {
    if (x === undefined)
      return acc
    else {
      let {data, pages} = await fakeAsyncOperation(x)
      return enqueue([...acc, data], [...xs, ...pages])
    }
  }
  return enqueue([], [page])
}

foo('root').then(pages => console.log(pages))

Output

[ 'root',
  'a',
  'b',
  'c',
  'd',
  'a/a',
  'a/a',
  'b/a',
  'c/a',
  'c/b',
  'c/c',
  'c/d',
  'b/a/a',
  'b/a/a/a' ]

Remarks

I'm happy that the foo function in my solution is not too far off from your original – I think you'll appreciate that. They both use an inner auxiliary function for looping and approach the problem in a similar way. async/await keeps the code nice an flat and highly readable (imo). Overall, I think this is an excellent solution for a somewhat complex problem.

Oh, and don't forget about circular references. There are no circular references in my dataset, but if page 'a' were to have pages: ['b'] and page 'b' had pages: ['a'], you can expect infinite recursion. Because this answer processes the pages serially, this would be a very easy to fix (by checking the accumulated value acc for an existing page identifier). This is much trickier (and out-of-scope of this answer) to handle when processing the pages in parallel.

2 Comments

Great answer and it's nice to see async and await becoming the norm. I assumed that OP's code is attempting to navigate a file-system so, if we ignore links (symlinks), circular references are probably not an issue. Also, your StackOverflow formatting skiils are off the chain!
Thanks for the feedback, @Phil ^_^
0

The issue here is mixing async/sync operations in if (resp.pages) branch. Basically you'll have to return promise from then callback if you want the promise chain to work as expected.

In addition to Phil's answer, if you want to execute pages in order/sequence

if (resp.pages) {
  var pages = resp.pages.split(',');
  return pages.reduce(function(p, page) {
    return p.then(function() {
        return doo(page);
    });
  }, Promise.resolve());
}

1 Comment

That's pretty cool and a good addition if OP's asyncOperation cannot execute multiple requests in parallel but you aren't combining the results in any way
-1

The issue is you don't return anything when there's pages.

function foo(filepath) {

  var resultArr = [];

  function doo(file, promises) {
        let promise = asyncOperation(file).then(resp => {
            resultArr.push(resp.data);
            if (resp.pages) {
                var pages = resp.pages.split(',');
                var pagePromises = [];
                pages.forEach(page => {
                    return doo(page, pagePromises);
                });

                //Return the pages
                return pagePromises;
            } else {
                return resultArr;
            }
        });

        //They want it added
        if ( promises ) { promises.push( promise ); }

        //Just return normally
        else { return promise; }
    }

    return doo(filepath);
}

1 Comment

This won't wait for the recursive async operations

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.