0

Got to love nodeJS and asynchronous nature! With that, I'm dumbfounded how to to continue bc I can't keep nesting promises and of course that's a not go so I'm throwing up my hands bc each step requires a completed action with data from the previous step.

This is what I'm trying to accomplish and code is below.

  1. A new college comes into /sessions/college
  2. After getting the value of that key, go find advisors that subscribe to that college.
  3. Get the FCM tokens for the subscribing advisors
  4. Haven't even gotten to this part obviously, but send a FCM notification to subscribers.
  5. Tada!

exports.newSessionNotifer = functions.database.ref('/sessions/college').onCreate((snap, context) => {
    const college = snap.val(); 
    var promises = [];
    var getAdvisors = admin.database().ref('colleges').child(college).once('value').then((snapshot) => {
        const people = snapshot.val();
        var advisors = Object.keys(people);
        return advisors;
    }).then((advisors) => {
        return advisors.forEach((token) => {
            var advisorToken = admin.database().ref('users').child(token).child('fcmtoken').child('token').once('value');
            return console.log(advisorToken);
        });
    });

    return Promise.all(promises).then((values) => {
        console.log(promises);
        return console.log('Hi');
    });
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
Ben
  • 138
  • 10
  • What did you intend to do with that `promises` array? – Bergi Oct 14 '18 at 19:27
  • [Don't use `forEach`!](https://stackoverflow.com/q/37576685/1048572) If `advisors` is not an array but a firebase snapshot thingy, convert it to an array first. – Bergi Oct 14 '18 at 19:28
  • Advisors “can” be an array so I want to accomodate the possbility of it being an array by just doing a forEach. – Ben Oct 14 '18 at 20:30
  • 1
    The point (as explained in the link) was that you cannot use `forEach` with a callback that does something asynchronous. Use `map` instead to create an array of promises. – Bergi Oct 14 '18 at 21:49
  • `I can't keep nesting promises` - thing is, you haven't nested promises - and if you really want "flat" code ... `async`/`await` to the rescue – Jaromanda X Oct 15 '18 at 07:45
  • @Jaromanda X yes my code doesn't have nesting but to accomplish the actual task, you must nest. – Ben Oct 15 '18 at 15:54

2 Answers2

2

You're on the right track. once() returns a promise, and it is the set of promises from repeated calls to once that must be collected and run with Promise.all().

exports.newSessionNotifer = functions.database.ref('/sessions/college').onCreate((snap, context) => {
    const college = snap.val();
    return admin.database().ref('colleges').child(college).once('value');
}).then(snapshot => {
    const people = snapshot.val();
    let advisors = Object.keys(people);
    let promises = advisors.map(token => {
        return admin.database().ref('users').child(token).child('fcmtoken').child('token').once('value');
    });
    return Promise.all(promises);
});

EDIT Editing again, this time with the OP's answer in hand. On style, I'm not sure what lint says, but my definition of bad nesting style is when a then() block contains another then() block. Also regarding style, my approach to making this stuff comprehensible is to build (and test) small functions, one per async task.

On structure, the OP's new answer unnecessarily chains a second block after return advisors. Since advisors isn't a promise, we can carry on from there with synchronous code. Also on structure, the OP's solution creates a series of promises -- two for each advisor (get advisor token and push) -- but these are not certain to complete unless Promise.all is applied and returned.

Summing all that, my advice would be as follows...

On create, get the advisors for the college, send each a message.

exports.newSessionNotifer = functions.database.ref('/sessions/{sessionID}/college').onCreate((snap, context) => {
    const college = snap.val(); 
    return advisorsForCollege(college).then(advisors => {
        let promises = advisors.map(advisor => sendAdvisorMessage(advisor, college));
        return Promise.all(promises);
    });
});

Advisors for a college are apparently the keys from that college object

function advisorsForCollege(college) {
    return admin.database().ref('colleges').child(college).once('value').then(snapshot => Object.keys(snapshot.val()));
}

Sending an advisor message means getting the advisors token and doing a push. Return a two-promise chain that does that...

function sendAdvisorMessage(advisor, college) {
    return tokenForAdvisor(advisor).then(token => {
        let title = `There's a new session for ${college}!`;
        let body = 'Go to the middle tab and swipe right to accept the session before your peers do!'
        return sendToDevice(token, title, body);
    });
}

Now we just need one to get an advisor's token and one to do a push...

function tokenForAdvisor(advisor) {
    return admin.database().ref('users').child(advisor).child('fcmtoken').child('token').once('value');
}

function sendToDevice(token, title, body) {
    const payload = { notification: { title: title, body: body } };
    return admin.messaging().sendToDevice(token, payload);
};

I think lint should report all of the foregoing as just fine, even with promise nesting warning turned on.

danh
  • 62,181
  • 10
  • 95
  • 136
  • I don't think `onCreate` does return a promise, does it? – Bergi Oct 14 '18 at 19:29
  • @Bergi: Every Cloud Function that needs to continue running after the closing `}` is executed needs to return a promise. Otherwise Cloud Functions doesn't know the function is still active, and may close/reuse its container. See for an example of `onCreate` returning a promise: https://firebase.google.com/docs/functions/database-events#handle_event_data – Frank van Puffelen Oct 14 '18 at 19:31
  • @FrankvanPuffelen Ok, still I understood the OP's code so that he wants his promise chain to be inside the `onCreate` callback – Bergi Oct 14 '18 at 19:33
  • 1
    I’d prefer the promise chain to be inside the onCreate callback bc the actions are tied to specifically to the function. – Ben Oct 14 '18 at 20:33
  • @danh more I think about it, the more I disagree that I'm on the right path. IE, 2nd return admin.database....once doesn't actually return the value of that key bc you just add a .then snapshot and guess what? That's nesting promises, errr. Sense I'm getting is that I'm going to need to break every piece "transaction" into a separate function and have 1 function call another. Holistically, that's what makes sense; practically, this seems kind of tedious and ridiculous to accomplish something I'd consider pretty straight fwd in other programming languages. – Ben Oct 15 '18 at 03:56
  • @Ben - I've made an edit that defines the chain inside the callback. It is the way I have built my hooks in google cloud functions, and doesn't nest promises (by my definition anyway.... for me, I call it nesting when a then block *contains* a then block. So its really not nesting promises, it's about not nesting thens – danh Oct 15 '18 at 15:12
  • @danh you didn't need to bc the promise is going to be a nested promise and that's a no go. Almost tempted to just remove nesting from lint to see what will happen – Ben Oct 15 '18 at 15:53
  • 1
    @Ben, well sorry I couldn't help. I don't understand your definition of nesting. Perhaps add that to your question for others. Also, you know, not nesting is a style thing, not really a no-go so much as a shouldn't-go. Maybe, for whatever isn't working, add some evidence of the failure to your OP, too. – danh Oct 15 '18 at 16:11
  • @danh so you brought up a great point and jumped right into disabling the promise nesting in lint and viola! Added my "answer", but going to accept your answer bc of your map suggestion and stylistic > necessity. Thanks again! – Ben Oct 16 '18 at 01:12
  • Loaded your edit and it's awesome, it's clean and straight and easy to follow and of course it doesn't nest! But for some reason, I can't get this line to return the actual token return admin.database().ref('users').child(advisor).child('fcmtoken').child('token').once('value'); It's baffling that if I add .then snapshot and get the value it works, but wout it, there's no value. I swear Firebase allows you to pull the value of the token key without a promise. @FrankvanPuffelen ? – Ben Oct 16 '18 at 18:48
  • @Ben - I don't know the real-time database too well, but I suspect that `once()` always resolves to a snapshot, and that snapshots have a way to give you the real data (`val()`, I guess), but are never themselves the data in the database. – danh Oct 16 '18 at 21:19
  • @danh yeah, who knows bc I knew then... I added .then snapshot to pull the value. It’s just annoying bc I think firebase even says that it’s possible and I’ve seen other code snippets doing the same. Maybe I’m hallicunating. – Ben Oct 16 '18 at 21:25
2

Thanks to danh, here's my final code. Comment/feedback away! I decided to disable the promise nesting option within lint and viola!

exports.newSessionNotifer = functions.database.ref('/sessions/{sessionID}/college').onCreate((snap, context) => {
const college = snap.val(); 
return admin.database().ref('colleges').child(college).once('value').then((snapshot) => {
    const people = snapshot.val();  
    let advisors = Object.keys(people);
    return advisors;
}).then((advisors) => {
    return advisors.map(advisor => {
            return admin.database().ref('users').child(advisor).child('fcmtoken').child('token').once('value').then((snapshot) => {
            const token = snapshot.val();
            const payload = {
                    notification: {
                    title: `There's a new session for ${college}!`,
                    body: 'Go to the middle tab and swipe right to accept the session before your peers do!'
                    }
            };
            return admin.messaging().sendToDevice(token, payload);
        });
    });
});
});
Ben
  • 138
  • 10
  • 1
    glad you got a solution. I made several style and structure comments in another edit to my answer (wasn't sure if I should edit here or there) – danh Oct 16 '18 at 02:18
  • That’s awesome, going to test it but assume it’ll work and admittedly, what I had referenced the idea of breaking everything up into functions, but of course had little to no idea on how to do that so thanks again! – Ben Oct 16 '18 at 04:25