-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ES6 and Let's Encrpyt #14
Conversation
Move to ES6 and starting on windows friendly let's encrypt
Added most comments to functions. Added send file to `doErrorResponse`
Added missing coma Added check to not generate SSL for disabled routes
@Irrelon could you please review changes? |
Fixed all parts, that made server crash Started implementing Let's encrypt (greenlock) Updated package.json to include necessary things to run server and removed postinstall
@Akxe Looks ok so far, cannot merge this if it doesn't work yet though. Other comments are inline in the code. |
package.json
Outdated
@@ -1,7 +1,19 @@ | |||
{ | |||
"name": "node-irrelon-router", | |||
"version": "0.1.0", | |||
"name": "node-akxe-router", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not change the name of the project unless you want to deploy this yourself to NPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my bad
package.json
Outdated
"name": "node-irrelon-router", | ||
"version": "0.1.0", | ||
"name": "node-akxe-router", | ||
"version": "0.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not reverse the version numbers unless you are starting a complete fork that will never get merged back in.
package.json
Outdated
"ssl-config": "^0.0.9" | ||
}, | ||
"scripts": { | ||
"start": "forever start -a --uid \"akxe-router\" router.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name change here as well.
package.json
Outdated
"scripts": { | ||
"start": "forever start -a --uid \"akxe-router\" router.js", | ||
"test": "node router.js", | ||
"stop": "forever stop akxe-router" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name change here as well.
router.js
Outdated
const fsAccess = require('fs-access') | ||
const mkdirp = require('mkdirp') | ||
const configFilePath = __dirname + '/config.json'*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this stuff is no longer needed, remove the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed a lot and probably missed that part my bad
router.js
Outdated
const mkdirp = require('mkdirp') | ||
const configFilePath = __dirname + '/config.json'*/ | ||
|
||
const greenLock = require('greenlock') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semicolons have been pulled from the code completely. Semicolons remove ambiguity in code and should be in there. Seen too many errors in code caused by not adding them in the past from developers who are not as familiar with the code as you or I might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically the IDE warns about semicolons missing.
routesFile: './exampleConfig.json' | ||
}) | ||
router.listen() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that given this is a complete re-write, some tests should be made to ensure this works. Including websocket tests as well as we are supposed to support them too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agreed, however I am unable to resolve implementation issues with greenlock, thus it is not working with SSL. And second problem is that I don't know how to create tests, if you create one, I can copycat the others.
Solves #11, #12 and #13
Most of your code is commented for references, but parts that I knew how to rewrite is rewrited. All parts that are to be done are commented with TODO and what should be done. If you could help me out with the part, that is left, I would be very much pleased. I failed.
Now solves only #12, and partialy solves #11 and #13