Node module using promises and the revealing module pattern

Question

Can someone give me some feedback on this pattern?

I am writing a node.js module that connects to a remote API, caches JSON, returns it as output. I have a cli wrapper script that uses the module, and then returns the output to a non-node app for a completely different purpose.

Am I using promises correctly here? Does anyone have any good module pattern + promises examples to look at? Is it proper to promisify a non-promise library and then wrap that library inside of a new promise when creating a function?

In this example saveJSON, cachedJSON, and refreshJSON all use fs or request so I am returning promises for those.

The lib is similar to this:

foo.js:

var Promise = require('bluebird')
var fs = Promise.promisifyAll(require("fs"))
var request = Promise.promisifyAll(require("request"));

function Foo() {
    var json = {}

    var emptyJSON = function() {
        return {
            "entries": {
                "vars": {}
            }
        }
    }

    var saveJSON = function(json) {
        return new Promise(function(resolve, reject) {
            fs.writeFileAsync('foo.json', JSON.stringify(json))
              .then(function() {
                  resolve(cachedJSON())
              })
              .catch (function(e) {
                  reject(e)
              })
        })
    }

    var cachedJSON = function() {
        return new Promise(function(resolve, reject) {
            fs.readFileAsync('./foo.json')
              .then(JSON.parse)
              .then(function(json) {
                  resolve(json)
              })
              .catch(SyntaxError, function(e){
                  // File exists, but has a syntax issue
                  reject("Invalid json")
              })
              .catch (function(e) {
                  // Return empty json on error
                  resolve(emptyJSON())
              })
        });
    }


    var refreshJSON = function() {
        return new Promise(function(resolve, reject) {
            request.getAsync('http://berkeleymapper.googlecode.com/svn-history/r101/trunk/web/foo.json').get(1)
                   .then(JSON.parse)
                   .then(saveJSON)
                   .then(function(json) {
                        resolve(json)
                   })
                   .catch (function(e) {
                       // Return empty json on error
                       resolve(emptyJSON())
                   })
             })
    }

    return {
        cachedJSON: cachedJSON,
        refreshJSON: refreshJSON
    }

}

module.exports = new Foo()

Where this could be utilized in a situation like this:

runFoo.js:

var foo = require("./foo")

// Refresh and return the latest json from remote
foo.refreshJSON() 
   .then(function(json) {
       console.log('### Refresh cached json with remote and return ###')
       console.log(json) 
   });

// Return only the cached json
foo.cachedJSON()
   .then(function(json) {
       console.log('### Return cached json only ###')
       console.log(json)
   });

Answer 1

Overall it looks pretty good, but I do have some notes:

• You export new Foo but your Foo function isn't really a constructor. It's just a function that returns an object. This works just fine (one of JS' oddities), but personally I'd just call the function foo - lowercase, not a constructor - and use module.exports = foo();.

• You're skipping most semicolons - and you're skipping them inconsistently. Even if it was super consistent, I'd still give you my standard "I prefer to add semicolons myself, rather than let JS 'fix' my code"-speech, but since it's inconsistent that goes double. It's one thing to insist on skipping semis, it's another to kinda, sorta mostly skip them.

  My usual speech is that since semicolons are required in some places, and since JS does semicolon insertion, then JS actually needs those semicolons. It's not that they're optional, it's that JS is trying to make sense of incomplete code. Personally, I'd just rather just write complete code.

• I'd probably prefer to tell the module where to store its "foo.json" file. From the outside, there's no indication of how it's caching things - much less that it (over)writes a file in the working directory. That's a nasty surprise.
  You could simply export your Foo pseudo-constructor function (rather than its return value), and then give it a path argument when creating it.

  foo = require("foo")("/path/to/some/dir"); // or a path to a specific file
  

• I'm on the fence about the emptyJSON thing. It's not really empty: There's a structure there. This implies that some other code of yours expects that structure to be there instead of doing proper error handling. But that's not your module's problem. I'd rather forward any errors.

  On the other hand, you've hard coded the URL to fetch, so your code already has some major external coupling. As long as that URL's there, it's not a big deal to also hard code a blank response - but should that URL actually be there? Depends on how generic you want this to be. You current code is almost perfectly generic in the sense that it could fetch and cache any given JSON document, so I'd be tempted to make the URL an argument to. But it would also necessitate that you remove emptyJSON and forward errors, since you no longer know what even an "empty" response looks like.

• Your flow is a little odd: saveJSON resolves its promise using cachedJSON. This means that you're first serializing the JSON, writing a file, then reading the file and deserializing the JSON, even though you have all of it in memory when you call saveJSON. It's just a very roundabout way of doing things. By the way, the json variable seems pointless.

• Most of all, I'd like a function that returns JSON whether or not it's cached. Especially since you "hide" errors by returning JSON in all cases. That basically means that if I call cachedJSON and there isn't any cache, I still get a JSON object. An object I'll have to dig into to figure out if it actually contains anything (which implies I know what to look for etc.). And even so, I don't know if it's "empty" because there's no cache yet, or because the request failed, or because the JSON was invalid. I'm completely in the dark.

  So I basically have to call refreshJSON exclusively. I'd always have to call it at least once to ensure that something's been cached. But a CLI script doesn't have an easy way of know if it's running for the first time (and needs to refresh) or the 234th time (and can try the cache). So net result is that I'm better off always calling refreshJSON and never bother with cachedJSON at all. Which, in turn, makes it all kinda pointless - you just give the the hard disk some unnecessary exercise (doubly so with the write-then-read thing).

  A caching proxy like this should primarily be transparent, with the option of going straight for the cache (and perhaps getting null or undefined back), or forcibly refreshing the cache, even if it exists. Of course, here it gets kinda tricky, since one of the 2 hard problems in programming is knowing when to invalidate your cache. Thankfully files are timestamped, so you could just use that plus a max age value to know if and when you'd need to refresh the cache. The max age could be (yet another, but optional) argument to the exported function.

Basically, I'd like to use the module like this:

var cache = require('foo')('/some/path/to/use', 'http://example.com/index.json', 3600);

// just give me the JSON; I don't care if it's cached or not
cache.getJSON().then(function (json) {
  // yay, data.
}).catch(function (err) {
  // handle errors here
});

That's of course presuming a very generic implementation. You could also just hard code the URL, file path, and max age - but your code wouldn't be very reusable. But that's fair too, of course. Depends on your needs.

As a side-note: You say your module is "similar to this". The code itself looks pretty complete, so I can't imagine what you're left out. But whatever's been left out, I obviously can't review (nor can anyone else). Hence the rule that questions must contain the real code to be on-topic for Code Review. As mentioned, your code looks pretty close to complete, so it's fine to review. But then wouldn't it have been easier to actually post the real code?