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

Create a basic HTTP server #1

Closed
hueniverse opened this issue Mar 11, 2015 · 21 comments · Fixed by #7
Closed

Create a basic HTTP server #1

hueniverse opened this issue Mar 11, 2015 · 21 comments · Fixed by #7
Milestone

Comments

@hueniverse
Copy link

Let's get started!

First, we need some basic structure: a lib folder, a package.json file, and an initial index.js. Since we will be using hapi, we need to include that in the project dependencies.

Second, create a basic hapi server on port 8000 which responds to /version requests and replies with a simple { "version": "0.0.1" } JSON payload. The version in the response should come from the package.json file.

Assignment due: 3/17

Got questions or need help? Post a comment.

@hueniverse hueniverse changed the title 1. Create a basic HTTP server Create a basic HTTP server Mar 11, 2015
@arb arb mentioned this issue Mar 11, 2015
@AdriVanHoudt
Copy link
Contributor

Maybe a standard way to post PR will be helpful in the future e.g. every PR for this assignment looks like [Assignment 1] My PR title?

@rodrigo-medeiros
Copy link

@AdriVanHoudt +1

@hueniverse
Copy link
Author

I'm ok with that. Generally, github links to this issue are enough for me.

@rutaihwa
Copy link
Contributor

[ Assignment #1 ] Basic HTTP server

@hueniverse
Copy link
Author

Since I can't provide this level of detail on every pull request, here are all the generally applicable comments collected as I am reviewing all the submissions:

  • The project structure should follow the same style of every other hapi module. This means package.json should include a main property that points to either index.js or lib/index.js. If the project includes a root index.js, it must not include any code other than require('./lib/index.js'). This makes searching for code easier later as well as keeps the entire application in one spot. I plan to get rid of the root index.js file for all the hapi modules I maintain soon as that extra file is useless.
  • Most of the submissions had some minors style issues. Pay attention to the empty lines requirement after function declaration as well as not declaring any local variable other than UpperCamelCase required modules and internals. See https://gist.github.com/hueniverse/a06f6315ea736ed1b46d for more information.
  • While not a worthwhile optimization in this example, there is no reason to have an endpoint that returns a static object response in which that object is constructed every request. The version payload could have been prepared in advance internals.response = { version: internals.package.version }.
  • If a callback includes an error, check for it! Some of you forgot to check for the error in server.start(err).
  • If there is no way to recover from an error and the process should abort, instead of writing custom code to console log it and then exit, just call Hoek.assert(!err, err).
  • No need to make up your own .gitignore. Just copy it from hapijs/hapi. Also, you only need a root ignore file, not one in every directory.
  • When calling a callback interface, better to add return before to make it stand out and avoid bugs where the callback is executed and then some more code at an undefined time. In this case, in the handler make sure you call return reply({ version: internals.package.version });.

@hueniverse
Copy link
Author

Some more:

  • You pull request is for this repo, not yours. This means that your package.json file should reference this URI, not your fork.
  • The package.json file can be assigned to either an UpperCamelCase variable (var Package = require('./package.json');) or to an internals variable but not just a module global per the style guide.
  • Some of you went the extra mile with your package.json files. Nicely done!
  • Bonus points for people who included a route description. If you did, better to move handler inside config when present.
  • Pay attention to the assignment specifics. Some of you had the wrong port, version, or specified localhost for no reason.
  • hapi.js uses BSD for all licenses.

As you can see, those submitting early get a more detailed personal code review. I go over the pull request in order and once I make a comment, I reflect it here for everyone else because repeating it over and over again isn't practical. It is important to read the full assignment notes and review your code even if I didn't comment on it. Pretty much every submission had issues.

@rutaihwa
Copy link
Contributor

@oluoluoxenfree if I understand correctly, for the first assignment, it was supposed to be a basic server. It could as well be started as server.start(), I also think that @hueniverse was giving his feedbacks going forward for future works as it will build on this.

@oluoluoxenfree
Copy link

@rutaihwa Sorry for deleting I meant to edit, thank you!

@oluoluoxenfree
Copy link

@rutaihwa How did you reference the issue? All I can find is this.

@AdriVanHoudt
Copy link
Contributor

@oluoluoxenfree just put any of the keywords and a reference to the issue in the description like closes #1

@oluoluoxenfree
Copy link

@AdriVanHoudt Thank you!

Sorry for cluttering up the issue with questions, won't happen again.

@rutaihwa
Copy link
Contributor

Thanks @oluoluoxenfree I had overlooked that my self, thanks for the question.

@rutaihwa rutaihwa mentioned this issue Mar 16, 2015
@AdriVanHoudt
Copy link
Contributor

@oluoluoxenfree np and this is issue is for questions so feel free to ask whatever you want. If you need general Hapi help check out the gitter https://gitter.im/hapijs/hapi or https://github.com/hapijs/contrib

@muhammad-saleh
Copy link

Thanks a lot for your time and this was very helpful.
But sorry why are we referencing the issues ?

@AdriVanHoudt
Copy link
Contributor

It makes it easier to find what PR is related to what issue and vica versa, not so much needed now but helps when dealing with multiple PR's and issues

@hueniverse hueniverse added this to the 0.0.1 milestone Mar 16, 2015
zoe-1 added a commit to zoe-1/university-dev that referenced this issue Mar 17, 2015
A hapijs server with the below characteristics:
- lib folder
- package.json file
  Package file defines hapijs as a project dependency.
- An initial index.js file
- Server is created on port 8000.
- A route accepting /version requests and replies
  { "version":"0.0.1" } json payload.
  Above version response comes from package.json file.
@MrBri MrBri mentioned this issue Mar 17, 2015
zoe-1 added a commit to zoe-1/university-dev that referenced this issue Mar 17, 2015
A hapijs server with the below characteristics:
- lib folder
- package.json file
  Package file defines hapijs as a project dependency.
- An initial index.js file
- Server is created on port 8000.
- A route accepting /version requests and replies
  { "version":"0.0.1" } json payload.
  Above version response comes from package.json file.
zoe-1 added a commit to zoe-1/university-dev that referenced this issue Mar 17, 2015
A hapijs server with the below characteristics:
- lib folder
- package.json file
  Package file defines hapijs as a project dependency.
- An initial index.js file
- Server is created on port 8000.
- A route accepting /version requests and replies
  { "version":"0.0.1" } json payload.
  Above version response comes from package.json file.
@hueniverse
Copy link
Author

Quick note about internals. The idea is not to declare any variable on the module global (e.g. the individual file you are working on). This includes the server variable most of you declared. While some were getting close to how it should be done with internals.server, that's not ideal. I'll modify the master branch to show how I would prefer to do it.

@AdriVanHoudt
Copy link
Contributor

@hueniverse thanks for the clarification! How come no example on hapijs.com does this? Because the examples are too trivial?

@hueniverse
Copy link
Author

@AdriVanHoudt probably because I didn't write them... :-)

@AdriVanHoudt
Copy link
Contributor

@hueniverse haha

@fyockm
Copy link

fyockm commented Mar 17, 2015

@hueniverse thanks for the clarification on internals.

@johnmanko
Copy link

I think I committed, PR's, and commented with 'closes #1' correctly. Can someone confirm?

zoe-1 added a commit to zoe-1/udev-client that referenced this issue Jan 19, 2016
* outmoded/university#1
* hapis v12
* node v4 and above.
* ES6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.