Comment branch #1
Conversation
| * @param {function} cb Callback to execute once the server has started. | ||
| */ | ||
| function start(cb) { | ||
| server.serve(process.env.PORT, '127.0.0.1'); // NOTE: needs to be process.en |
There was a problem hiding this comment.
'127.0.0.1' should be removed.
There was a problem hiding this comment.
This is required by the native-dns server implementation.
There was a problem hiding this comment.
ooo, that sucks, then we should make it a process.env var if we put this guy in a container, that will not be the correct addr to listen too.
There was a problem hiding this comment.
hmm are we not using latest native-dns? looks like they merged PR with this change in
tjfontaine/node-dns#88
|
@anandkumarpatel what is this PR? all it's doing it tabbing all the code in the file... |
| * @param {object} res Response object when the error occurred. | ||
| */ | ||
| function error(err, buff, req, res) { | ||
| error(err.stack); |
There was a problem hiding this comment.
you need to end the request req.end() on errors.
There was a problem hiding this comment.
You're right, reading up on how to indicate different types of errors in the DNS response (it's not as easy as putting in a status code, haha).
There was a problem hiding this comment.
something like
https://github.com/CodeNow/navi/blob/master/lib/error.js#L16
There was a problem hiding this comment.
DNS uses a specialized format that is not compatible with HTTP. So I have to set specific bits in the response datagram so clients will handle it appropriately.
There was a problem hiding this comment.
cool, just read a up on it a bit, just logging might be good for now.
|
just a side note: I know @tjmehta has been promoting making everything more OOP with constructers and prototypes and what not and since we are writing this from scratch (like navi) might want to write it that way now (because it will never change later :P ) |
| * @return {string} The remote ip address for the request. | ||
| */ | ||
| function getRequestIP(req) { | ||
| return req._socket._remote.address; |
There was a problem hiding this comment.
this info will be in the req.headers lets avoid using _* methods in other peoples modules (we hit issues with this before since people can make _* change as a patch and next time we npm install we will get that version)
There was a problem hiding this comment.
So req in this context, while similar to express, is nothing like express. The request object here is is being created specifically from native-dns. From investigating the the request object this seemed to be the only way to get the requestor's ip address. I'll look into it more though, as I also dislike the underscore attributes.
|
@bkendall its so I can comment on the file. |
|
huh.. okay... |
I am using this to put comments on specific lines of code.