Proper calling of next callback in express get()

Question

I have an express server running on nodejs. I have a file structure that I want to turn into a list of links. I've included my recursive file structure functions.

file structure functions

function fileTreeWrapper (root, callback) {
    output = recursiveFileTree(root);
    console.log(output);
    callback();
}

function recursiveFileTree (root) {
    fs.readdir('./' + root, function (err, files) {
        if(err) throw err;
        var structure = '';
        var folders = '';
        var tests = '';

        structure += '<ul>';
        
        for(var i in files) {
            if(files[i].indexOf('.') === -1) {
                folders += '<li id="folder">' + root + '/' + files[i] + '">' + files[i].charAt(0).toUpperCase() + files[i].slice(1);
                folders += recursiveFileTree(root + '/' + files[i]);
                folders += '</li>';
            } else if (files[i].indexOf('.js') > -1) {
                tests += '<li id="file"><a href="' + root + '/' + files[i] + '">' + files[i] + '</a></li>';
            }
        }
        structure += folders;
        structure += tests;
        structure += '</ul>';
        return structure;
    });
}

I call router.get like so:

router.get('/', [function (req, res, next) {
    fileTreeWrapper('rootFolder', function() {
        next();
    });
}, function (req, res) {
    res.render('Files & Folders', {
        title: 'Main',
        localTitle: 'Choose a File',
        result: output
    }, function (err, html) {
        if(err) throw err;
        res.send(html);
        res.end();
    });
}]);

However, when I run this, my page shows up blank. My jade template for rendering looks like this:

h1= title
h3= localTitle
div !{result}

Am I using the next() callback inappropriately? Why would this not work?

Answer 1

There's quite a bit wrong with what you were originally doing. I re-wrote all of your code to help it make more sense. Hopefully this helps.

function recursiveFileTree(root, callback) {
  fs.readdir('./' + root, function (err, files) {
    if (err) { return callback(err); }
    try {
      var structure = '';
      var folders = '';
      var tests = '';
      structure += '<ul>';
      files.forEach(function (file) {
        if (file.indxOf('.') === -1) {
          folders += '<li id="folder">' + root + files[i] + '">' + file.charAt(0).toUpperCase() + file.slice(1);
          folders += recursiveFileTree(root + '/' + file);
          folders += '</li>';
        } else {
          tests += '<li id="file"><a href="' + root + '/' + file + '">' + file + '</a></li>';
        }
      });
      structure += folders;
      structure += tests;
      structure += '</ul>';
      callback(null, structure);
    } catch (err) {
      return callback(err);
    }
  })
};


router.get('/', function (req, res) {
  recursiveFileTree('rootFolder', function (err, structure) {
    if (err) { throw err; }
    res.render('fileListingView', {
      title: 'Main',
      localTitle: 'choose a File',
      result: structure
    });
  });
});

Notes:

• It didn't look like you even needed to use a middleware function (the function that gets next as an argument). Think of middleware like a pipeline. The req goes through each middleware (section of the pipeline) in order until it hits your final handler (the one that finally calls res.send, res.json, or res.render).

• Calling res.render is all you need to send a view down to the client.

• The first argument to res.render is the name of the view template it should render. Files & Folders doesn't make much sense passed in there. You would need a view file named that and I don't think your file name can have spaces or an ampersand.

• The second argument to res.render are the variables you want to make available to your view template. Calling res.render with just those two arguments implicitly sends the rendered view to the client. There's no need to pass in a callback function as the third argument unless you really need to get a string version of the rendered view for use on the server.

• next is the function you call from middleware functions to let express know that you're ready for it to continue down the middleware pipeline. You don't really have a need for a middleware function here, but if you did then any errors you catch should be passed to the next function as the first argument. Express knows that if you pass a value to next then it should jump to the express error handler and render an error message view for the user.

  I'm fairly certain that throwing errors inside middleware functions or the final request handler function will still be caught by express though.

• You cannot ever ever ever return a value from a function if it is asynchronous. This is precisely the reason you need to pass callback functions everywhere. You're passing in a function so that node can call it later when the data you want is ready, then node can invoke your callback and pass in the data you asked for. Meanwhile, your app moves on and is able to process other code while it waits. You might want to look into promises because they clean up the callback hell where you end up calling asynchronous functions inside callbacks for other asynchronous functions, creating a confusing sideways pyramid of hell like this:

  asyncFunction1(function (err, result) {
    if (err) return handleErr(err);
    asyncFunction2(function (err, result) {
      if (err) return handleErr(err);
      asyncFunction3(function (err, result) {
        if (err) return handleErr(err);
        asyncFunction4(function (err, result) {
          if (err) return handleErr(err);
          asyncFunction5(funciton (err, result) {
            if (err) return handleErr(err);
            // do something useful
          });
        });
      });
    });
  });
  

  Promises will make that code much more readable. Promises also make it so you don't have to check for errors in every single callback. Look them up ;). Here's an example of what that same code would look like using promises:

  asyncFunction1()
    .then(function (result) {
      return asyncFunction2();
    })
    .then(function (result) {
      return asyncFunction3();
    })
    .then(function (result) {
      return asyncFunction4();
    })
    .then(function (result) {
      return asyncFunction5();
    })
    .then(function (result) {
      // do something useful
    })
    .catch(function (err) {
      // Will be called if ANY of the above generate an error.
    });
  

---

That's most of the feedback I have for now and it's probably a lot for you to process so I'll leave it at that :)