0

I have a function that returns a promise (using Q), and the notifications don't seem to happen at the right time. The onFulfilled and onRejected callback work as intended, but the progress callback doesn't fire until after async.whilst() finishes running, and fires everything at once.

This is the function

function generateSentences(data, num, options) {
    var deferred = Q.defer();
    const markov = new Markov(data, options);
    markov.buildCorpus()
        .then(() => {
            var count = 0;
            async.whilst(
                function () { return count < num; },
                function (callback) {
                    markov.generateSentence()
                        .then(result => {
                            console.log("Count: " + count);
                            deferred.notify(count / num); //update progress
                            count++;
                            callback(null);
                        }, (err) => {
                            deferred.reject(err.toString());
                            count++;
                        });
                },
                function (err, n) {
                    //PROGRESS EVENTS DON'T HAPPEN UNTIL HERE
                    deferred.resolve(generatedSentences); //finish
                }
            );
        }, (err) => console.log(err));
    return deferred.promise;
}

and this is using the promise

function generateScript() {
    fs.readdir(parser.videoBasePath, function (err, files) {
        parseFiles(files, parser.parse).then((a) => {
            console.log("Total Lines: " + fullScript.length + "\n");
            fullScript = _.shuffle(fullScript);
            markov.generateSentences(fullScript, 20).then((data) => {
                console.log(data);
            }, (err) => {
                console.log(err);
            }, (progress) => {
                console.log(progress);
            });
        });
    });
}

I've read some threads like this saying I need to wrap a setTimeout around the notify(), but it doesn't seem to affect anything.

Community
  • 1
  • 1
Brian Le
  • 159
  • 10
  • For diagnosis, if you register a progress callback inside `generateSentences()` immediately after `deferred` is created, does it behave the same or differently from your existing progress callback? – Roamer-1888 Dec 29 '16 at 06:48
  • Perhaps this is why mixing Promises and async is considered to be a *Bad Thing To Do* – Jaromanda X Dec 29 '16 at 09:28
  • @Roamer-1888, just tried this, it behaves the same as the existing callback.I was unaware that mixing promises and async was bad practice, this is my first time attempting a nodejs project. That being said, I attempted another solution from one of the answer that eliminates the needs for async, but still suffers that same problem – Brian Le Dec 29 '16 at 13:32
  • That's odd, I can't explain the behaviour, also can't understand why @JaromandaX's answer doesn't work - his code looks good. Maybe there are issues at the [microtask vs macrotask](http://stackoverflow.com/questions/25915634/) level. My best suggestion would be to ditch `async.js` and `Q`/`.notify()` in favour of the much less sophisticated approach of passing a "notify" callback to `generateSentences()`. – Roamer-1888 Dec 29 '16 at 22:03

1 Answers1

0

I've read that Promises + async.js don't mix (but can't find anything to say as much!!), and I can't really see why that should be an issue in this case to be honest

Having said that, the code you've shown seems to be possible without async, so try this to see if the progress works any better

function generateSentences(data, num, options) {
    var deferred = Q.defer();
    const markov = new Markov(data, options);
    const genSentence = count => markov.generateSentence()
        .then(result => {
            console.log("Count: " + count);
            deferred.notify(count / num); //update progress
            if (count < num) {
                return genSentence(count + 1);
            }
        });
    markov.buildCorpus()
        .then(() => genSentence(0))
        .then(() => deferred.resolve(generatedSentences)) //finish
        .catch(err => deferred.reject(err.toString()));
    return deferred.promise;
}

At the very least, the code is (in my opinion) a little cleaner anyway

Jaromanda X
  • 53,868
  • 5
  • 73
  • 87
  • Definitely cleaner, thanks. I've attempted your solution [hee](https://gist.github.com/anonymous/bad9c379a371a9529eff8033ce22cfa9), but I still seem to end up with the same issue. One thing I've noticed is if I comment out the 'return genSentence' line to only get to loop to run once, the progress callback works properly. This leads me to believe notify() doesn't like the recursive nature of how we implemented it, and how async.js implement it? – Brian Le Dec 29 '16 at 13:28
  • Never been a "fan" of the Promise progress - some early implementations had it, I even modified a pretty well regarded Promise implementation to include the "progress chain" - but in the end, never really saw the need for it - which seems to be the way of Promises in general – Jaromanda X Dec 29 '16 at 13:44
  • Would there be any workarounds for my issue though? Promise progress seemed to be the only thing that fit my use case. – Brian Le Dec 29 '16 at 14:33
  • off the top of my head, event dispatch/listen would be useful for progress notifications – Jaromanda X Dec 29 '16 at 15:24