Yet Another Todo API

Question

I've been on an adventure trying to learn NodeJs and put together a stack that I'd be happy to put into production if one day my team said to me "Let's build it in Node!". What we have here is your standard "Todo Webservice". Any and all feedback is appreciated, because I had never written more than 10 lines of JS prior to a month ago.

I do have a few specific concerns:

• The setup for the TodoControllerSpec feels kind of verbose. Can it be simplified?
• If I forget to call .catch((err)=> throw err) on a test, I can get silent failures. Is there a better way to handle unit tests when promises are involved?
• Have I structured my middleware in an expected way? This kind of felt right, but I'm unsure if there are any expected patterns for Express middleware to implement.

The "main" packages I'm using are Express, LokiJs, Mocha, and Chai, but for completeness, here's my package.json file so you know all the libraries I've imported.

 "devDependencies": {
 "chai": "^3.5.0",
 "mocha": "^3.1.2",
 "node-mocks-http": "^1.5.4",
 "proxyquire": "^1.7.10",
 "sinon": "^1.17.6",
 "sinon-as-promised": "^4.0.2",
 "sinon-chai": "^2.8.0"
 },
 "dependencies": {
 "body-parser": "^1.15.2",
 "express": "^4.14.0",
 "handlebars": "^4.0.6",
 "hbs": "^4.0.1",
 "http-auth": "^3.1.1",
 "lokijs": "^1.4.1",
 "q": "^1.4.1"
 }

index.js

var express = require('express'),
 bodyParser = require('body-parser');
var app = express();
app.use(bodyParser.json());
app.set('view engine', 'hbs');
var auth = require('http-auth');
var basic = auth.basic({file: __dirname + "/data/users.htpasswd"});
// uncomment to password protect the whole api
// app.use(auth.connect(basic));
// use this as middleware just for routes we want
var requireAuth = auth.connect(basic);
// require authentication for all post requests
app.post('*', requireAuth);
var root = require('./controllers/root'),
 todo = require('./controllers/todo'); 
app.get('/', root.index);
app.get('/todo', todo.findAll);
app.get('/todo/:id', todo.findById);
app.post('/todo', todo.insert);
app.post('/todo/:id', todo.update);
// Order is important here, the error handler needs to be after all the routes for it to fire correctly.
var errorHandler = require('./middleware/errorHandler');
app.use(errorHandler.handle);
app.listen(1337,
 function () {
 console.log('Web api is listening on port 1337');
 });

./middleware/errorHandler.js

A global error handler that will return a 500 Internal Server Error status should anything go wrong.

exports.handle = function (error, request, response, next) {
 response.status(500);
 response.format({
 json: function () {
 response.send();
 },
 html: function () {
 response.render('error');
 }
 });
 next(error);
}

./test/errorHandlerSpec.js

var mocha = require('mocha'),
 expect = require('chai').expect,
 httpMocks = require('node-mocks-http');
var errHandler = require('../middleware/errorHandler');
describe('errorHandler', function () {
 describe('when json requested', function () {
 var request, response;
 beforeEach(function () {
 request = httpMocks.createRequest({
 headers: {
 Accept: 'application/json'
 }
 });
 responseOptions = { req: request };
 response = httpMocks.createResponse(responseOptions);
 });
 it('returns status 500 internal error', function () {
 var error = new Error('Fake Error');
 errHandler.handle(error, request, response, () => { });
 expect(response.statusCode).to.equal(500);
 });
 it('should log');
 });
 describe('when html requested', function () {
 var request, response;
 beforeEach(function () {
 request = httpMocks.createRequest({
 headers: {
 Accept: 'text/html'
 }
 });
 responseOptions = { req: request };
 response = httpMocks.createResponse(responseOptions);
 });
 it('should get the error view', function () {
 const error = new Error('Fake Error');
 errHandler.handle(error, request, response, () => { });
 expect(response._getRenderView()).to.equal('error');
 });
 it('should log');
 });
});

./views/todo.hbs

I like to be able to easily browse my API's in the browser, so my controller returns the following Handlebars view when HTML is requested from the http://localhost/todo/ endpoint.

<h2>Backlog</h2>
<ul>
 {{#each this}}
 {{#if done}}
 <li><strike>{{title}}</strike></li>
 {{else}}
 <li>{{title}}</li>
 {{/if}}
 {{/each}}
</ul>

./controllers/todo.js

Is responsible for talking to the repository and returning the correct http responses.

var q = require('q');
var todoDb = require('../repositories/todoRepository');
exports.findAll = function (request, response, next) {
 return todoDb.findAll()
 .then(function (data) {
 response.format({
 json: () => response.send(data),
 html: () => response.render('todo', data)
 });
 }).catch(function (err) {
 next(err);
 });
}
exports.findById = function (request, response, next) {
/*
 todo: pass back html when requested via response.render()
*/
 return todoDb.findById(request.params.id)
 .then(function (data) {
 if (!data) {
 response.sendStatus(404);
 return;
 }
 response.send(data);
 }).catch(function (err) {
 next(err);
 });
}
exports.insert = function (request, response, next) {
 return todoDb.insert(request.body)
 .then((result) => {
 response.status(201).send(result);
 }).catch((err) => {
 next(err);
 });
}
exports.update = function (request, response, next) {
 return todoDb.update(request.params.id, request.body)
 .then((item) => {
 if (!item) {
 response.sendStatus(404);
 return;
 }
 response.send(item);
 }).catch(function (err) {
 next(err);
 });
}

./test/TodoControllerSpec.js

var mocha = require('mocha'),
 expect = require('chai').expect,
 httpMocks = require('node-mocks-http'),
 proxy = require('proxyquire'),
 sinon = require('sinon'),
 sinonAsPromised = require('sinon-as-promised'),
 sinonChai = require('sinon-chai'),
 expect = require('chai').use(sinonChai).expect;
var todo = require('../controllers/todo');
describe('TodoController', () => {
 var repoStub;
 var request, response, testData;
 beforeEach(() => {
 // mock http request & response
 request = httpMocks.createRequest();
 response = httpMocks.createResponse();
 //set up a clean, fake db
 testData = [
 {
 "title": "Setup test site on different port.",
 "done": true,
 "meta": { "revision": 0, "created": 1479043435802, "version": 0 },
 "$loki": 1
 },
 {
 "title": "Finish the todo api",
 "meta": { "revision": 0, "created": 1479051068239, "version": 0 },
 "$loki": 2
 }];
 var findAll = sinon.stub().resolves(testData);
 var findById = sinon.stub().resolves(testData[0]);
 var update = sinon.stub();
 var insert = sinon.stub();
 repoStub = {
 findAll: findAll,
 findById: findById,
 update: update,
 insert: insert
 }
 todo = proxy('../controllers/todo', { '../repositories/todoRepository': repoStub });
 })
 describe('findAll', () => {
 describe('html requested', () => {
 beforeEach(() => {
 request = httpMocks.createRequest({
 headers: {
 Accept: 'text/html'
 }
 });
 var responseOptions = { req: request };
 response = httpMocks.createResponse(responseOptions);
 });
 it('should return todo view', () => {
 return todo.findAll(request, response)
 .then(() => {
 expect(response._getRenderView()).to.equal('todo');
 }).catch((err) => {
 throw err;
 })
 });
 it('should return todo items', () => {
 return todo.findAll(request, response)
 .then(() => {
 expect(response._getRenderData()).to.deep.equal(testData);
 }).catch((err) => {
 throw err;
 });
 });
 });
 describe('json requested', () => {
 beforeEach(() => {
 request = httpMocks.createRequest({
 headers: {
 Accept: 'application/json'
 }
 });
 var responseOptions = { req: request };
 response = httpMocks.createResponse(responseOptions);
 });
 describe('when successful', () => {
 it('should return all items', () => {
 return todo.findAll(request, response)
 .then(() => {
 var actual = response._getData();
 expect(actual).to.deep.equal(testData);
 }).catch((err) => {
 throw err
 });
 });
 it('should return a 200 ok', () => {
 return todo.findAll(request, response)
 .then(() => {
 expect(response.statusCode).to.equal(200);
 }).catch((err) => {
 throw err;
 });
 });
 });
 });
 describe('when database fails', () => {
 it('calls next', () => {
 var expectedError = new Error('Failed to connect to database');
 repoStub.findAll.rejects(expectedError);
 var next = sinon.spy();
 return todo.findAll(request, response, next)
 .then(() => {
 expect(next).to.have.been.calledOnce;
 expect(next).to.have.been.calledWith(expectedError);
 }).catch((err) => {
 throw err;
 });
 })
 });
 });
 describe('findById', () => {
 describe('when item is found', () => {
 beforeEach(() => {
 request._setParameter('id', 1);
 });
 it('should return 200 ok', () => {
 return todo.findById(request, response)
 .then(() => {
 expect(response.statusCode).to.equal(200);
 }).catch((err) => {
 throw err;
 });
 });
 it('should only return the expected item', () => {
 return todo.findById(request, response)
 .then(() => {
 var actual = response._getData();
 expect(actual).to.deep.equal(testData[0]);
 }).catch((err) => {
 throw err;
 });
 });
 });
 describe('when item is not found', () => {
 beforeEach(() => {
 request._setParameter('id', 0);
 });
 it('should return 404 not found', () => {
 repoStub.findById.resolves(null);
 return todo.findById(request, response)
 .then(() => {
 expect(response.statusCode).to.equal(404);
 }).catch((err) => {
 throw err;
 });
 });
 });
 describe('when database fails', () => {
 it('should call next', () => {
 var expectedError = new Error('Failed to connect to database');
 repoStub.findById.rejects(expectedError);
 var next = sinon.spy();
 return todo.findById(request, response, next)
 .then(() => {
 expect(next).to.have.been.calledOnce;
 expect(next).to.have.been.calledWith(expectedError);
 }).catch((err) => {
 throw err;
 });
 });
 });
 });
 describe('insert', () => {
 describe('when successful', () => {
 var body = { name: 'spy on the insert' };
 beforeEach(() => {
 request = httpMocks.createRequest({ body: body });
 repoStub.insert.resolves(body);
 });
 it('inserts to the database', () => {
 return todo.insert(request, response)
 .then(() => {
 expect(repoStub.insert).to.have.been.calledOnce;
 expect(repoStub.insert).to.have.been.calledWith(body);
 }).catch((err) => {
 throw err;
 });
 });
 it('returns the new todo item in the response body', () => {
 return todo.insert(request, response)
 .then(() => {
 expect(response._getData()).to.deep.equal(body);
 }).catch((err) => {
 throw err;
 });
 });
 it('returns 201 created', () => {
 return todo.insert(request, response)
 .then(() => {
 expect(response.statusCode).to.equal(201);
 }).catch((err) => {
 throw err;
 });
 });
 it('includes a uri pointing to the new resource');
 });
 describe('when failed', () => {
 it('calls next', () => {
 var expectedError = new Error('Failed to save to database.');
 repoStub.insert.rejects(expectedError);
 var next = sinon.spy();
 return todo.insert(request, response, next)
 .then(() => {
 expect(next).to.have.been.calledOnce;
 expect(next).to.have.been.calledWith(expectedError);
 }).catch((err) => {
 throw err;
 });
 });
 });
 });
 describe('update', () => {
 describe('when item is found', () => {
 var postData = { title: 'Foo', done: 'true' };
 beforeEach(() => {
 request = httpMocks.createRequest({ body: postData });
 response = httpMocks.createResponse({ req: request });
 repoStub.update.resolves(postData);
 });
 describe('when successful', () => {
 it('includes updated todo item in response body', () => {
 return todo.update(request, response)
 .then(() => {
 expect(response._getData()).to.deep.equal(postData);
 }).catch((err) => {
 throw err;
 });
 });
 it('returns 200 ok', () => {
 return todo.update(request, response)
 .then(() => {
 expect(response.statusCode).to.equal(200);
 }).catch((err) => {
 throw err;
 });
 });
 });
 describe('when failure', () => {
 it('calls next', () => {
 var expectedError = new Error('Failed to save to database.');
 repoStub.update.rejects(expectedError);
 var next = sinon.spy();
 return todo.update(request, response, next)
 .then(() => {
 expect(next).to.have.been.calledOnce;
 expect(next).to.have.been.calledWith(expectedError);
 }).catch((err) => {
 throw err;
 });
 });
 });
 });
 describe('when item is not found', () => {
 beforeEach(() => {
 request._setParameter('id', 0);
 repoStub.update.resolves(null);
 });
 it('should return 404 not found', () => {
 return todo.update(request, response, () => { })
 .then(() => {
 expect(response.statusCode).to.equal(404);
 }).catch((err) => {
 throw err;
 });
 });
 });
 });
});

./repositories/todoRepository.js

Queries data from the LokiJs database. It also wraps LokiJs's callback style in a Promise, so the promise chain really starts here.

var loki = require('lokijs'),
 db = new loki('data/todo.json');
var Q = require('q');
exports.findAll = function () {
 var deferred = Q.defer();
 db.loadDatabase({}, function () {
 try {
 deferred.resolve(db.getCollection('todo').data);
 }
 catch (err) {
 deferred.reject(err);
 }
 });
 return deferred.promise;
}
exports.findById = function (id) {
 var deferred = Q.defer();
 db.loadDatabase({}, function () {
 try {
 var item = db.getCollection('todo')
 .findOne({ '$loki': id * 1 });
 deferred.resolve(item);
 }
 catch (err) {
 deferred.reject(err);
 }
 });
 return deferred.promise;
}
exports.update = function (id, item) {
 var deferred = Q.defer();
 db.loadDatabase({}, function () {
 try {
 var items = db.getCollection('todo');
 var doc = items.findOne({ '$loki': id * 1 });
 if (!doc) {
 deferred.resolve(null);
 return deferred.promise;
 }
 doc.title = item.title;
 doc.done = item.done;
 items.update(doc);
 db.save();
 deferred.resolve(item);
 } catch (err) {
 deferred.reject(err);
 }
 });
 return deferred.promise;
}
exports.insert = function (item) {
 var deferred = Q.defer();
 try {
 db.loadDatabase({}, function () {
 var doc = db.getCollection('todo').insert(item);
 db.save();
 deferred.resolve(doc);
 });
 } catch (err) {
 deferred.reject(err);
 }
 return deferred.promise;
}

./test/todoRepositorySpec.js

var mocha = require('mocha'),
 proxy = require('proxyquire'),
 sinon = require('sinon'),
 sinonChai = require('sinon-chai'),
 expect = require('chai').use(sinonChai).expect,
 sinonAsPromised = require('sinon-as-promised');
var todo = require('../repositories/todoRepository');
describe('TodoRepository', () => {
 const testData = [
 {
 "title": "Setup test site on different port.",
 "done": true,
 "meta": { "revision": 0, "created": 1479043435802, "version": 0 },
 "$loki": 1
 },
 {
 "title": "Finish the todo api",
 "meta": { "revision": 0, "created": 1479051068239, "version": 0 },
 "$loki": 2
 }];
 var collectionStub = {
 data: testData,
 findOne: (query) => {
 for (var i = 0; i < testData.length; i++) {
 if (query.$loki === testData[i].$loki) {
 return testData[i];
 }
 }
 },
 insert: () => { },
 update: () => { }
 }
 var saveSpy = sinon.spy(),
 getCollectionStub = sinon.stub().returns(collectionStub);
 var postData;
 beforeEach(() => {
 postData = { title: 'foo', done: true };
 saveSpy = sinon.spy();
 var lokiStub = function loki(filename) {
 this.loadDatabase = function (options, callback) {
 callback();
 };
 this.getCollection = getCollectionStub;
 this.save = saveSpy;
 };
 todo = proxy('../repositories/todoRepository', { 'lokijs': lokiStub });
 });
 describe('update', () => {
 var updateSpy = sinon.spy();
 before(() => {
 updateSpy = sinon.spy(collectionStub, 'update');
 })
 it('calls update', () => {
 return todo.update(1, postData)
 .then(() => {
 expect(updateSpy).to.have.been.calledOnce;
 }).catch((err) => {
 throw err;
 });
 });
 it('saves to the database', () => {
 return todo.update(1, postData)
 .then(() => {
 expect(saveSpy).to.have.been.calledOnce;
 }).catch((err) => {
 throw err;
 });
 });
 });
 describe('insert', () => {
 var insertSpy = sinon.stub();
 before(() => {
 insertSpy = sinon.stub(collectionStub, 'insert', () => {
 return postData;
 });
 })
 it('inserts the item', () => {
 return todo.insert(postData)
 .then(() => {
 expect(insertSpy).to.have.been.calledOnce;
 expect(insertSpy).to.have.been.calledWith(postData);
 }).catch((err) => {
 throw err;
 });
 });
 it('saves to the database', () => {
 return todo.insert(postData)
 .then(() => {
 expect(saveSpy).to.have.been.calledOnce;
 }).catch((err) => {
 throw err;
 });
 });
 it('returns the new document', () => {
 return todo.insert(postData)
 .then((result) => {
 expect(result).to.deep.equal(postData);
 }).catch((err) => {
 throw err;
 })
 });
 });
});

• m0meni: Regarding: "If I forget to call .catch((err)=> throw err) on a test, I can get silent failures. Is there a better way to handle unit tests when promises are involved." Look into nodejs.org/api/process.html#process_event_unhandledrejection. Node from v7? I think will automatically log to console if there's an unhandled rejection, but in earlier versions you can just add a listener yourself.
• RubberDuck: Yes @AR7 it does log to the console, but the test will pass. When you've got lots of tests, it's easy to miss that log in the middle of the test results. So, "silent" is a poor choice of words. It's not silent, but also not as loud as I'd like.
• m0meni: I did a little research since I don't personally use mocha as my test runner (jest is awesome), but it seems like mocha will automatically fail without a catch block if you return your promise in the test. So you can get rid of all your catch blocks. Also I just realized, the way you're handling your errors is completely useless lol. The result is the same as if you didn't even catch in the first place. Read this
• RubberDuck: Umm... no @AR7. I tried that. Without the catch, the test passes. It also passes if I don't return the promise from the test.
• RubberDuck: It seems my problem isn't mocha, it's chai. stackoverflow.com/a/40842060/3198973

Answer 1

Okay I'm going to be a bit more comprehensive here since I've already read your code quite a few times, and I'm gonna assume that you are on node v6

The setup for the TodoControllerSpec feels kind of verbose. Can it be simplified?

If you mean all the require calls you have to do...unfortunately not as far as I know. As a side note, I really like jest for this reason because it basically has almost all of your dependencies built into it, and additionally has built in code coverage and async support. You can see the API here.

If I forget to call .catch((err)=> throw err) on a test, I can get silent failures. Is there a better way to handle unit tests when promises are involved?

So once again the best way to handle this is to return your promise, and if there's a rejection, mocha itself will catch it, and fail. When I said that

.catch((err) => {
 throw err;
});

was useless, I meant that if you throw an error inside a catch block, it doesn't escape the scope of the promise; instead, it's just propagated to either the next catch block, or is left unhandled. Thus, the result of the above code, is the same as not catching at all.

This post I linked shows how you could make the error occur outside the promise, but I wouldn't recommend it.

Regarding chai-as-promised that's yet another dependency you'd need, and you can solve your problem without it.

Again, as a side note, in your actual application code, you use catch properly like here

.catch(function (err) {
 next(err);
});

instead of rethrowing the error, which is practically a no-op, you did something with it that wouldn't have happened otherwise.

Have I structured my middleware in an expected way? This kind of felt right, but I'm unsure if there are any expected patterns for Express middleware to implement.

Looks good to me :)

Other Points

Q, Deferred, and Promises

Don't use Q. First, it's not the best promise implementation, and second you're using it for the deferred anti-pattern.

Regarding the first point, ideally you don't require a promise library, and just use the built in Javascript promises. However, if you truly want to require another implementation, bluebird is the absolute best promise library there is, and even beats the promises that come by default in node both performance wise and in terms of having a more extensive API.

Regarding the second point, you're writing

exports.findById = function (id) {
 var deferred = Q.defer();
 db.loadDatabase({}, function () {
 try {
 var item = db.getCollection('todo')
 .findOne({ '$loki': id * 1 });
 deferred.resolve(item);
 }
 catch (err) {
 deferred.reject(err);
 }
 });
 return deferred.promise;
}

but the more idiomatic version is:

exports.findById = function (id) {
 return new Promise(function(resolve, reject) {
 db.loadDatabase({}, function () {
 try {
 const item = db.getCollection('todo')
 .findOne({ '$loki': id * 1 });
 resolve(item);
 } catch (err) {
 reject(err);
 }
 })
 });
}

var vs. const vs. let

I only mention this because I see you wrote const testData in only one place, but nowhere else.

The long story short on the best practices I've seen is:

• Don't use var anymore because it's not block-scoped like let and const
• Use const for everything that you can. const only means that you can't reassign a variable using =. You can still mutate the variable if it's an object or array by adding/changing keys or pushing new values respectively.
• Use let when you need to use reassignment.

Lots more detail on this here

Conclusion

Your code looks mostly good, and other than some inconsistent style or extra syntactic sugar you could use, there's nothing else that objectively needs fixing.

Since you have a C# background, I would recommend you look into TypeScript. I wrote a blog post on a similar tool called Flow a while back that describes the benefits of why'd you'd want to use any tool similar to Flow/TypeScript.

I would also recommend you familiarize yourself more with the new es6 features that are coming out as of late. Here's a quick resource https://github.com/DrkSephy/es6-cheatsheet, and here's a comprehensive one http://exploringjs.com/es6/index.html#toc_ch_variables

Finally you might want to take some time to read through http://eslint.org/docs/rules/, and use eslint yourself. It's really helpful for understanding common mistakes people make and is a good tool to have overall.

• RubberDuck: Thank you very much for the excellent answer. I'll take another look at the .catch() thing, but this was pretty much what I had to do to get the tests working properly. So, I'm obviously missing some subtle point. I'm also going to try using Node's built in promises. I didn't know they were there, so thanks again!
• m0meni: @RubberDuck yeah it's really weird to me that you have to both catch and return. I also don't know what quirk requires you to have a catch in the first place, but hopefully you get it sorted out.
• Dan: As an aside, OP could save a lot of time by using Bluebird and running promisifyAll across the entire lokijs module, thereby eliminating the need for the promise wrappers altogether

Answer 2

Here's something that bothers me about this code. You have all the flexibility of Express, yet you've structured you're project like an Asp.Net project.

/
|--index.js
|
|--Views/
| |--todo.hbs
|
|--Controllers/
| |--todo.js
|
|--Repositories/
| |--todoRepository.js
|
|--Middleware/
| |--...
| 
|--Test/
 |--...

You, yourself, wrote an answer on Software Engineering talking about how this is cargo cult programming.

[A project's structure] should tell me that it's a store front, or a time off request system, or whatever. The high level structure and architecture should tell us about what this thing is, not how it was implemented.

It would be much nicer to structure the project so that it's immediately evident what this software does.

/
|--index.js
|
|--Todo/
 |--controller.js
 |--repository.js
 |--index.hbs

This structure removes the awkward and inconsistent naming you had previously, giving you import statements that look like this.

var todoDb = require('../Todo/repository');

and this

var root = require('./Root/controller'),
 todo = require('./Todo/controller'); 

instead of this

var todoDb = require('../repositories/todoRepository');

and this

var root = require('./controllers/root'),
 todo = require('./controllers/todo'); 

Grouping your objects by functionality like this both makes it really stupid easy to name things and grants you this great kind of "namespacing". Anytime you need to modify the "todo" functionality in any way, you'll know exactly where to look for the file.

There is a small downside to this however, you'll need to explicitly tell Express to check these new directories for views.

app.set('view engine', 'hbs');
app.set('views', [__dirname + '/todo', __dirname + '/views']);