Add bin/www, a templating engine, and error pages

[openjck]

These changes are related in that they work together to move us closer to the setup that express-generator[1] recommends.

[1] https://www.npmjs.com/package/express-generator

[openjck]

Two small things:

• We aren't currently suppressing log messages in test output. So even though the 404 test passes, a nasty error message about the 404 is printed to the console.
• All tests pass, but a genuine error message also appears and the tests don't exit. I'm not sure what caused that, but I'm looking into it.

[openjck]

Scratch that. Tests are passing now.

[Faeranne]

Since the focus of this server is API, I feel like forcing MVC on it will only make things more difficult. And it doesn't seem worth it for one page.

[groovecoder]

Is there an issue we're fixing or a feature we're planning to add that needs MVC?

[openjck]

Introducing MVC did take some work. I was also tepid about it. Introducing the database will be easier if we follow this convention, especially because sequelize encourages the pattern. Thankfully these changes really aren't changing any significant logic. The biggest change is that views and controllers have been put into separate subdirectories.

[darkwing]

I think I agree with @mrmakeit. We only have two views (index and hook), and this PR feels to me like a bit of overcomplication. Can I read up somewhere on why you think this is the right move? I think I need some selling.

[Faeranne]

I'm also concerned that we are treating hook as a view. It feels like it should be more of an endpoint or api than a view, since we aren't sending any formated document, just a single object JSON.

[openjck]

I'm also concerned that we are treating hook as a view. It feels like it should be more of an endpoint or api than a view, since we aren't sending any formated document, just a single object JSON.

The hook isn't a view. It's in the controllers directory.

[openjck]

I think I agree with @mrmakeit. We only have two views (index and hook), and this PR feels to me like a bit of overcomplication. Can I read up somewhere on why you think this is the right move? I think I need some selling.

I can see how this might sound big. The changes might even look substantial.

First and foremost, the goal is not to organize views. The goal is to introduce models. Improving the organization of the codebase is going to be important as we introduce database logic, migrations, and other related infrastructure. It's what the sequelize module recommends, and for good reason. I started to introduce the database before working on this but quickly realized that it was going to be much more challenging without a solid foundation.

We could try to introduce models, migrations, and other database infrastructure without following a standard MVC pattern, but I'm not sure why we would want to. Moving to MVC is easy (the code is already there) and continuing to use that pattern should have no real overhead. Don't let the diff scare you. All this really does is move views and controllers into separate subdirectories.

Aside from that, the only other changes are the addition of the bin/www script (which is the just script that the Express team recommends and generates with express-generator) and the addition of a templating language (which we are going to need to use environment variables in pages as Luke correctly recommends).

We can keep the repository flat, and avoid re-organizing views and controllers into separate subdirectories, but I'm not sure what we would gain by doing that when the cost is so small and the benefits are so substantial as we introduce more complicated database infrastructure.

[Faeranne]

@darkwing's comment led me to believe it was a view. Should have double checked that, thanks.

As for using MVC at all, it makes sense when you plan to have several views, but at the moment we have exactly one. If we intend to introduce more views, this change makes sense.

Without extra views, we are adding extra code that will not be used, and encourage patters that don't apply. And we can easily introduce models without using a full MVC stack.

Either way, I don't think models particularly benifit this project. Can you point to where we would need a full model over a few values?

PS: your link points to the express generator, not the sequelize article about why to use MVC.

[openjck]

As for using MVC at all, it makes sense when you plan to have several views, but at the moment we have exactly one. If we intend to introduce more views, this change makes sense.

We have more than one view now (the homepage view and the error view). We may also add other pages in the future. We've talked about adding reports, for example.

We could stay with out current architecture for now and refactor the code as pages are added. Preparing for that eventuality now may be easier, especially when the upfront cost is so small (the code for moving views and controllers into different subdirectories is already written). I would rather do things right now than spend twice as much time cleaning things up later.

PS: your link points to the express generator, not the sequelize article about why to use MVC.

I did a double-take at first, too. The link is to the sequelize/express-example repository. It's their example for using sequelize in Express.

[openjck]

Just to be clear, there are really three things happening here:

• Views and controllers are moved into separate folders (the only true MVC part)
• A templating system is added (we need it to use environment variables in templates)
• The bin/www script is added to handle common server errors like ports already being in use (we should have been using this from the start)

It was easiest to do everything at once. The diff may look scary, but the MVC changes are just moving files into different directories. Everything else is stuff we should have been doing anyway.

[openjck]

Maybe we should reset the stage. I could have done a better job communicating earlier.

I definitely understand the hesitation about these changes. I was hesitant to make them. Code is a liability. We shouldn't add more unless we absolutely need to. I completely agree that we shouldn't use a pattern for no good reason.

I resisted making these changes at first and only later saw that the benefits outweighed the costs. The bin/www script does important safety checks that we should really take advantage of, the templating engine is honestly already overdue (we should have used one with the GA addition), and even though moving views and controllers into different directories is a small change, I would feel much more confident moving forward if we keep things organized and follow the pattern the sequelize team recommends.

I hate adding unnecessary code, but feel good about these changes because they all serve important purposes.

[Faeranne]

I can see adding the www script and templating, but I'm still on the fence on adding MVC. Perhaps splitting this into two PR's would help make the discussion on these easier.

[openjck]

I can see adding the www script and templating, but I'm still on the fence on adding MVC. Perhaps splitting this into two PR's would help make the discussion on these easier.

Definitely. I agree that combining everything here is less than ideal. It makes evaluating the individual changes a lot more annoying than it needs to be. I considered renaming the views folder (originally html) in a different pull pull request but came to feel that it should be tied to the introduction of the templating engine (since templates are not truly HTML). Doing rendering in a separate controllers directory could easily be moved into another pull request.

[Faeranne]

If the controllers is moved to another pr, I can easily recommend this for merge.

[openjck]

Updated! :smile:

The changes now only include: the bin/www script, a templating engine, and (along with the templating engine) better handling of HTTP errors.

[Faeranne]

Now that I've taken a good look at bin/www it seems like it may be better to just put it inside app.js, since it keeps referencing outside files. It feels like bin/ files should be a bit more self-contained.

Either way, this looks good to go!

[openjck]

We can definitely consider merging bin/www into app.js. The two have a close relationship that definitely raises some flags. One benefit of keeping them separate is that the lower-level server "plumbing" and higher-level application logic would be separated. That may be why express-generator separates them by default.

[openjck]

What do you think about the newly-revised changes, @darkwing?

[Faeranne]

Makes sense. r+

[openjck]

In fact, the name of this branch has outlived the discussion. Let me open another PR entirely.