4

I'm trying to create a MVC pattern with Node.js Express, but this seems to be a quite impossible task when doing asynchronous code.

Case in point: I'm fetching results from a NeDB database in the model, like so:

controllers/database.js

// Database management stuff with NeDB
var data = {};
db.findOne({ _id: 1 }, (error, doc) => {
    if (error) console.error(error);
    data = doc;
}

And now, I'm going to use it in a controller called dbcontroller.js:

var nedb = require('../models/nedb.js');

module.exports.list = function(req, res, next) {
    res.render('listpage', {
        data: nedb.data,
    });

And in the routes/routes.js file:

var express = require('express');
var router = express.Router();
var dbcontroller = require('../controllers/dbcontroller.js');
// Other controllers...

router.get('/list', dbcontroller.list);
// Other route definitions...

module.exports = router;

Cute arrangement, isn't it? Now, sure enough, the NeDB findOne() function is asynchronous, so it's going to execute after module.exports, and that won't do. The solution that came to mind was, of course, putting the module.exports definition inside the findOne() callback!

db.findOne({ _id: 1 }, (error, doc) => {
    if (error) console.error(error);
    module.exports.data = data;
}

I'm sure this is an anti-pattern somewhere, but it works. Or does it? The real drama starts now.

When it's called by the controller, the findOne() function also finishes after all of the controller logic, meaning nedb.data will be undefined. I'll have to do some crazy stunt now...

database.js

module.exports.execute = function (callback) {
    db.findOne({ _id: 1 }, (error, doc) =>
    {
        if (error) console.error(error);
        module.exports.data = doc;
        callback();
    });
}

dbcontroller.js:

nedb.execute(export);

function export() {
    module.exports.list = function(req, res, next) {
        res.render('listpage', {
            data: nedb.data,
        });
    };
};

And it gets worse. The routes.js file now can't find dbcontroller.list, presumably because it's still defined after the route asks for it. Now, will I have to start putting route definitions in callbacks too?

My point is, this whole asynchronicity thing seems to be completely taking over my code structure. What was previously tidy code now is becoming an ugly mess because I can't put code in the place it belongs. And that's just one async function, NeDB has lots of asynchronous functions, and while that's great, I don't know if I can handle it if just findOne() is already giving me such a headache.

Maybe I don't really have a choice and I must mess up my code in order for it to work? Or maybe I'm missing something really basic? Or maybe libraries like async or promises will do what I'm looking for? What do you think?

Improve this question

2 Answers 2

Reset to default
5

In order for async code to work, you need to be notified of when it's done by either a callback, or promises, or something else. To make your examples above work, all you need to do is call res.render after you're sure that data is defined. To do this, you can either return a promise from your database fetches, or pass it a callback. I would suggest promises, since they're much easier to use because they help avoid "callback hell", and Node has had native support for them for a long time now.

controllers/database.js

module.exports.fetch = function() {
  return new Promise(function(resolve, reject) {
    db.findOne({ _id: 1 }, (error, doc) => {
      if (error) reject(error);
      resolve(doc);
   });
}

Then, in your controller, you just need to call fetch (or however you end up defining your database fetching code) before you render your response.

dbcontroller.js

var nedb = require('../models/nedb.js');

module.exports.list = function(req, res, next) {
  nedb.fetch().then((data) => {
    res.render('listpage', {
        data: nedb.data,
    });
  }).catch((err) => { /* handle db fetch error */ });
}

The thing to keep in mind is that express doesn't expect you to call res.render right away. You can execute a lot of async code, and then render the response when it's done.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

This isn't exactly a great example of how promises are "much easier" than callbacks. You can do exactly the same thing w/ a callback in less code. I didn't -1 you, but honestly your answer would be better if you either leave off the style opinion or else demonstrate why it's better.
If you notice the question, it's not about callbacks vs. promises. I wasn't trying to prove that promises were "much easier." But the javascript community is pretty agreed that they help with "callback hell." That's why they have native support. The question was asking what async approach would lead to cleaner code, and I simply gave my opinion. Yeah, obviously, if you only have two functions, callbacks would be less code. But the question is about building MVC for an app.
All that's why I didn't -1 you. I just think your answer would be better w/o bringing the debate in and making unsupported assertions.
I quote from the question. "Or maybe libraries like async or promises will do what I'm looking for? What do you think?" In response to this simple question, I answered that "I suggest using promises." Would you have been upset if I had used callbacks instead, and noted how incompatible my code is with contemporary practices? Just saying, I don't feel like this is productive.
I'm not upset, man, I'm just suggesting a way to improve your answer. If you don't want to do it, so be it. But it seems like it would've been easier for you to take my advice by e.g. adding an example of why the Promise approach is better for his scenario, or ignore it, than engaging in an unproductive defense against my comment. I didn't say you were wrong about Promises, just that your example would have been better if you'd shown this new developer why your assertion was true.
|
3

It is usually bad practice to set module data inside of callback functions since require loads modules synchronously. Passing ES6 Promises between modules is a possible option to eliminate the need for excessive callbacks.

database.js

module.exports = new Promise(function (resolve, reject) {
    db.findOne({ _id: 1 }, (error, doc) => {
        if (error) reject(error);
        resolve(doc);
    });
});

dbcontroller.js

var nedb = require('../models/nedb.js');

module.exports.list = nedb.then(data => {
    return (req, res, next) => { //return router callback function
        res.render('listpage', { data }) //ES6 shorthand object notation
    }
}).catch(/* error handler */);

Further guidance can be found in this answer: Asynchronous initialization of Node.js module

Improve this answer

3 Comments

The problem I'm having is that the code being require()d is somehow working asynchronously. I thought this wouldn't happen since require() is synchronous (as stated in the answer you shown me) and it really makes sense for node.js to load the whole module before starting code in the actual file... But still, the piece of code in database.js that loads data to the controller is running parallel to the controller itself. Is my node.js somehow broken?
Best way I can explain it: your database.js is exporting a function object which is loaded synchronously into dbcontroller.js without executing (otherwise the line nedb.execute(export) would return cannot find property 'execute' of undefined). When the database.js function is executed with a callback in dbcontroller.js, the database fetch is still asynchronous despite the function already being defined. Thus, the need for several callbacks/promises across modules.
It's crazy that I can't trust modules to fully load before the current code. I wonder how module creators do to avoid having users doing random callbacks for their modules to work.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.