1

Morning All

I've been scouring S.O. for the past couple of days trying to find an answer I understood/was applicable to my situation, and have now admitted defeat, primarily due to my lack of comprehension around Promises (promise-n00b). So I'm basically putting out a plea for some help with my example posted below. I think it's fairly explanatory what I'm doing but in case it isn't, I'm trying to:

  • Apply some synchronous config to the server
  • Load a Logger module and instantiate a new of instance of it as logger, and then run some necessary checks (e.g. do we have a log file?) before returning an indicator to say that it loaded successfully (either a boolean or the logger object itself)
  • Then pass that logger to the utils (they need it to log)
  • Finally call the callback passed in by the scripts run by npm start and npm test

Obviously there's a lot more going on in the real world but I'd tried to distil the code down to the bit I don't get, namely the Promise chain.

Finally, as a long term user of callbacks who has never struggled to comprehend them (brain must work differently perhaps) do any of you dudes have any pearls of wisdom likely to cause a lightbulb moment?

Many thanks in advance and yes I know this code as it stands doesn't/won't work ;-)

Code as follows:

server.js

var Promise = require('bluebird');
var fs = Promise.promisifyAll(require('fs-extra'));

var boss = {
start: function(callback) {
    var p = new Promise.try(function() {
        console.log('start');
    })
    .then(boss.applyConfig)
    .then(boss.startLogger)
    .then(function(lggr) {
        console.log('logger setup complete: ' + lggr);
        boss.logger = lggr;
        return lggr;
    })
    .then(boss.loadUtils)
    .finally(function() {
        if (callback) callback();
    })
    .catch(function(err) {
        console.log.call(null, '\033[31m' + err.stack + ' \033[39m');
    });
},
applyConfig: function() {
    console.log('applying config');
    return 'config';
},
startLogger: function() {
    console.log('starting logger');
    var Logger = require('./Logger')(fs, Promise);
    var logger = new Logger();
    return new Promise(function(resolve, reject) {
        var result = logger.init();

        if (result) {
            resolve(logger);
        }
        else {
            reject(logger);
        }
    });
},
loadUtils: function(logger) {
    console.log('loading utils with ' + logger);
    boss.logger.log('loading utils with ' + logger);
    return 'utils';
}
};

boss.start(function(){
   console.log('done');
});

Logger.js

module.exports = function(fs, Promise) {

var Logger = function(options) {
    this.filePath = (options && options.filePath) ? options.filePath : __dirname + '/../log/';
    this.filename = (options && options.filename) ? options.filename : 'all.log';
};

Logger.prototype.init = function() {
    var logger = this;
    console.log('logger.init');
    return new Promise(function(resolve) {
        new Promise.resolve(logger.checkForLogFile()).then(function(exs) {
            console.log('exs', exs);
            return exs;
        });
    })
};

Logger.prototype.log = function(msg) {
    // requires that the log file exists and that we have a stream to it (create stream function removed)
    console.log.call(null, '\033[36mLogging: ' + msg + ' \033[39m');
};

Logger.prototype.checkForLogFile = function() {
    var logger = this;
    fs.existsAsync(logger.filePath + logger.filename).then(function (exists) {
        console.log('exists', exists);
        return exists;
    });
};

return Logger;
};
nick-brown
  • 31
  • 2
  • 8
  • 1
    Watch out for the [deferred anti pattern](stackoverflow.com/questions/23803743/what-is-the-deferred-antipattern-and-how-do-i-avoid-it) - you don't need `new Promise` all around - simply return promises and use `then` for chaining. Moreover - in `checkForLogFile` you're not returning the promise (you're missing a `return` before your `fs.existsAsync` - worse - `existsAsync` doesn't work with promises since `exists` is broken - if you must use it use `statAsync` instead - otherwise avoid it as it is susceptible to race conditions (file is created between the exists check and the next command?) – Benjamin Gruenbaum Dec 01 '14 at 11:36
  • 1
    Also - return `p` rather than accept a callback in your promised methods (like `start`) – Benjamin Gruenbaum Dec 01 '14 at 11:37
  • Thanks for the heads-up re existsAsync Benjamin - that explains a lot! Going through your other comments now. – nick-brown Dec 01 '14 at 11:42
  • Re your 2nd comment (about returning p in start), I deliberately didn't want to do this as I didn't want the calling scripts to have to understand Promises - would you regard this as an acceptable use case for a callback or am I getting completely the wrong end of the stick? – nick-brown Dec 01 '14 at 11:50
  • Ok, reading your other comments, still don't get it - when you say "simply return promises" what is this in actual implementation (I thought that was what I was doing). – nick-brown Dec 01 '14 at 11:54
  • Re statAsync - thanks - following code works as expected: `Logger.prototype.checkForLogFile = function() { var logger = this; return fs.statAsync(logger.filePath + logger.filename).then(function (statObj) { console.log('statObj', statObj); return statObj; }); };` – nick-brown Dec 01 '14 at 11:55

1 Answers1

0

First up - thanks Benjamin :) And secondly, how amazingly surprising that all this would now work within 30 mins of posting on S.O. after a couple of days prior effort ;-)

So yes - golden takeaway nugget is KISS!

Following code now works correctly and logs in the correct order as follows:

start
applying config
starting logger
logger.init
statObj { dev: 2067,
  mode: 33204,
  ...
  ctime: Thu Nov 27 2014 11:40:34 GMT+0000 (GMT) }
exists true
logger setup complete: [object Object]
loading utils with [object Object]
Logging: loading utils with [object Object] 
done

server.js

var Promise = require('bluebird');
var fs = Promise.promisifyAll(require('fs-extra'));

var boss = {
start: function(callback) {
    var p = new Promise.try(function() {
        console.log('start');
    })
    .then(boss.applyConfig)
    .then(boss.startLogger)
    .then(function(lggr) {
        console.log('logger setup complete: ' + lggr);
        boss.logger = lggr;
        return lggr;
    })
    .then(boss.loadUtils)
    .finally(function() {
        if (callback) callback();
    })
    .catch(function(err) {
        console.log.call(null, '\033[31m' + err.stack + ' \033[39m');
    });
},
applyConfig: function() {
    console.log('applying config');
    return 'config';
},
startLogger: function() {
    console.log('starting logger');
    var Logger = require('./Logger')(fs, Promise);
    var logger = new Logger();
    return logger.init();
},
loadUtils: function(logger) {
    console.log('loading utils with ' + logger);
    boss.logger.log('loading utils with ' + logger);
    return 'utils';
}
};

boss.start(function(){
   console.log('done');
});

Logger.js

module.exports = function(fs, Promise) {

var Logger = function(options) {
    this.filePath = (options && options.filePath) ? options.filePath : __dirname + '/../log/';
    this.filename = (options && options.filename) ? options.filename : 'all.log';
};

Logger.prototype.init = function() {
    var logger = this;
    console.log('logger.init');
    return logger.checkForLogFile().then(function(statObj) {
        var exists = (typeof statObj === 'object');
        console.log('exists', exists);
        return logger;
    });
};

Logger.prototype.log = function(msg) {
    // requires that the log file exists and that we have a stream to it (create stream function removed)
    console.log.call(null, '\033[36mLogging: ' + msg + ' \033[39m');
};

Logger.prototype.checkForLogFile = function() {
    var logger = this;
    return fs.statAsync(logger.filePath + logger.filename).then(function (statObj) {
        console.log('statObj', statObj);
        return statObj;
    });
};

return Logger;
};
nick-brown
  • 31
  • 2
  • 8