Skip to content
This repository has been archived by the owner on Apr 8, 2019. It is now read-only.

[Assignment 1] Basic server #7

Merged
merged 7 commits into from
Mar 17, 2015
Merged

Conversation

AdriVanHoudt
Copy link
Contributor

Closes #1

var Hapi = require('hapi');

var internals = {
pkg: require('./package.json')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the overview submissions should be using the hapi style guide, which specifies 4 spaces for rather than tabs for indents

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the head up, I'll have to change IDE settings for that, no problem
And I didn't know that guide existed O_o

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no probs 😄

return reply({ version: internals.pkg.version });
},
config: {
description: 'Returns the version of the server'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch! If you have config, it's cleaner to put handler inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by config? And yes once the routes get more complex the handlers will be moved out of the config.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe he means that it makes sense to do

server.route({
    method: 'GET',
    path: '/version',
    config: {
        description: 'Returns the version of the server',
        handler: function (request, reply) {
            return reply({ version: internals.pkg.version });
        }
    }
});

I typically move my handlers to a file ./handlers.js, so it looks like this in my index.js

server.route({
    method: 'GET',
    path: '/version',
    config: handlers.version
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, yes I also move them to a separate place. Thanks for clearing that up!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hueniverse is that a general rule? To put handler in config if it is present? It will probably be cleaner once the handler is in a separate place

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I like to do. More readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hueniverse hueniverse modified the milestone: 0.0.1 Mar 16, 2015
assertion // updated license // updated gitignore file (taken from hapi
repo)
@@ -0,0 +1 @@
module.exports = require('./lib/index.js');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to export anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just require('./lib/index.js'); then? Or even require('./lib') although i'm not familiar with what it will do then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok found it in the docs will patch

hueniverse pushed a commit that referenced this pull request Mar 17, 2015
@hueniverse hueniverse merged commit 2252b51 into outmoded:master Mar 17, 2015
@AdriVanHoudt
Copy link
Contributor Author

@hueniverse awesome thanks! If you want them squashed next time let me know :p

hueniverse pushed a commit that referenced this pull request Mar 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a basic HTTP server
4 participants