6

I'm trying to recursively fetch all the comments for a Hacker News story using their Firebase API. A story has a kids property, which is an array of IDs representing comments. Each comment can have its own kids property, which points to its children comments, and so on. I want to create an array of the entire comment tree, something that looks like:

[{
  'title': 'comment 1', 
  'replies': [{
    'title': 'comment 1.1'
  }, {
    'title': 'comment 1.2'
    'replies': [{
      'title': 'comment 1.2.1'
    }]
  }]
}]

I thought I could do this using the following function:

function getItem(id) {
    return api
        .child(`item/${id}`)
        .once('value')
        .then((snapshot) => {  // this is a Firebase Promise
            let val = snapshot.val()

            if (val.kids) {
                val.replies = val.kids.map((id) => getItem(id))
            }

            return val
        })
}

And then be notified once the entire comment tree has been fetched by using:

getItem(storyId)
    .then(story => {
      // The story and all of its comments should now be loaded
      console.log(story)
    })

What ends up happening is Promise.all().then() fires once the first level of comment promises have resolved (which makes sense since all of the commentPromises have resolved.) However, I want to know once all the nested promises have resolved. How can I do this?

Steven Mercatante
  • 24,757
  • 9
  • 65
  • 109
  • When you call `getItem(storyId)` the property `story.kids` is already getting processed inside `getItem()` via `if(val.kids)` isn't it? In that case do you need to again process them via `var commentPromises = story.kids.map(id => getItem(id))`? – Vivek Athalye Jan 28 '17 at 03:08
  • You're right; that part is redundant. I'll edit my question. – Steven Mercatante Jan 28 '17 at 03:31

2 Answers2

7

There is no need for a wrapper promise as in other answers; that is the "explicit promise constructor anti-pattern". Streamlining a bit more, you can do the following:

function getItem(id) {
  return api
    .child(`item/${id}`)
    .once('value')
    .then(snapshot => {
      const val = snapshot.val();
      return Promise.all((val.kids || []).map(getItem))
        .then(kidsVals => {
          val.replies = kidsVals; 
          return val; 
        });
      );
    });
}

There is also no need for any explicit rejection handling. Rejections will propagate up naturally to the top level (assuming that's what you want).

Community
  • 1
  • 1
  • i was just working on something similar, but yours is even better. nice work torazaburo ^_^ – Mulan Jan 28 '17 at 04:29
  • Very nice. I especially like how you got rid of the `if (kids.vals)` conditional by using `(val.kids || [])`. Thanks. – Steven Mercatante Jan 28 '17 at 04:36
  • Indeed neat and clean.. +1 – Vivek Athalye Jan 28 '17 at 05:36
  • This can be detuned slightly such that the outer and inner `.then().then()` pairs are each reduced to a single `.then()` (the first `.then()` is wholly synchronous in both cases). The result will be less pretty but more performant (every `.then()` creates and returns a new Promise). – Roamer-1888 Jan 29 '17 at 08:18
  • 1
    @Roamer-1888 Thanks for your comment. I've revised my solution accordingly. –  Jan 30 '17 at 23:22
3

IMP: Please refer torazaburo's answer. It's much better than mine.

=========

I think this should work:

function getItem(id) {
    return new Promise(function(resolve, reject) {
        api
        .child(`item/${id}`)
        .once('value')
        .then((snapshot) => {  // this is a Firebase Promise
            let val = snapshot.val()

            if (val.kids) {
                let replies = val.kids.map((id) => getItem(id))
                Promise.all(replies).then(replies => {
                    val.replies = replies // <<<<<< try this
                    resolve(val) // we want to resolve val, not val.replies
                }, err =>{
                    reject(err)
                })
            } else {
                resolve(val)
            }
        })
        .catch((err) => { // if there was some error invoking api call we need to reject our promise
            reject(err);
        })
    }
}

Edit: Setting val.replies inside then

Vivek Athalye
  • 2,974
  • 2
  • 23
  • 32