2
\$\begingroup\$

I have a NodeJS package, staticize, which takes an object of HTTP routes -> file locations, then serves them dynamically on each request.

Could I get some feedback on the latest rewrite of the main file? (Also available on GitHub here)

var fs = require('fs');
var mime = require('mime');

module.exports = function(app, routes) {
    // loop through routes
    for (var index in routes) {
        // closure: route is the HTTP request route, path is the file path
        (function (route, path) {
            app.get(route, function(req, res) {
                // create a readable stream
                var stream = fs.createReadStream(path, {encoding: 'utf8'});

                stream
                    .once('error', function() {
                        res.status(500).end('Server error.');
                        console.error('ERROR (staticize): Could not open readable stream to ' + path);
                    })
                    .once('readable', function() {
                        // create headers
                        var headers = {
                            'Content-Type': mime.lookup(path)
                        };

                        // write head then pipe response
                        res.writeHead(200, headers);
                        stream.pipe(res);
                    });
            });
        })(index, routes[index]);
    }
};

It is called simply like this:

staticize(app, {
    '/foo': '../test/foo/bar.txt',
    '/foo/more': '../test/foo-extra/bar.txt'
});

I've tried to perfect it as much as possible, but I may have overlooked some things.

\$\endgroup\$
  • \$\begingroup\$ I'm a bit confused by "for index in routes" - shouldn't it be "for route in routes"? \$\endgroup\$ – netpoetica Oct 21 '14 at 20:54
1
\$\begingroup\$

It looks pretty good and definitely works, but I would approach this a little bit differently and try to avoid that closure - it's a little bit difficult to read and it's also unnecessary. You can use Object.keys if you want to be able to use an object for your routes parameter.

module.exports = function(app, routes) {
    // loop through routes
    var len = routes.length,
        keys = Object.keys(routes),
        // Define path and stream outside of the loop, at the top
        // of the module as private vars and reuse (they are
        // hoisted by JavaScript anyway.
        url,
        path,
        stream;

  // Use a while loop here because it doesn't matter which
  // order you setup these routes in. This is the least verbose.
  while(len--){
    url = keys[len];
    app.get(url, function(req, res) {
        path = routes[url];
        // create a readable stream
        stream = fs.createReadStream(path, {encoding: 'utf8'});

        stream
        .once('error', function() {
            res.status(500).end('Server error.');
            console.error('ERROR (staticize): Could not open readable stream to ' + path);
        })
        .once('readable', function() {
            // create headers
            var headers = {
                'Content-Type': mime.lookup(path)
            };

            // write head then pipe response
            res.writeHead(200, headers);
            stream.pipe(res);
        });
    });  
  }
};

Personally, I would use an array with named properties instead of an Object for my routes, but key/value is OK. Something like this would allow you to just do a routes.forEach() and have direct access without having to use Object.keys:

staticize(app, [
    {
        url: '/foo',
        path: '../test/foo/bar.txt'
    }, {
        url: '/foo/more',
        path: '../test/foo-extra/bar.txt'
    }
]);

module.exports = function(app, routes) {
  // Use a forEach here instead of a loop
  routes.forEach(function(route){
    app.get(route.url, function(req, res) {
        // Save the route since we use it three times in here
        var path = route.path;

        // create a readable stream
        var stream = fs.createReadStream(path, {encoding: 'utf8'});

        stream
        .once('error', function() {
            res.status(500).end('Server error.');
            console.error('ERROR (staticize): Could not open readable stream to ' + path);
        })
        .once('readable', function() {
            // create headers
            var headers = {
                'Content-Type': mime.lookup(path)
            };

            // write head then pipe response
            res.writeHead(200, headers);
            stream.pipe(res);
        });
    });  
  });
};
\$\endgroup\$
  • \$\begingroup\$ Thanks for the advice! I'll definitely implement some of the changes. \$\endgroup\$ – Brendan Oct 22 '14 at 2:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.