Skip to content
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

Clarified use of host, use path.combine for paths and lodash.defaults for config. Expose config. #72

Merged
merged 2 commits into from Feb 6, 2017

Conversation

Projects
None yet
2 participants
@dblock
Copy link
Collaborator

commented Feb 6, 2017

No description provided.

@dblock dblock force-pushed the dblock:test-server-config branch 5 times, most recently from aa21ffb to 7e25038 Feb 6, 2017

@dblock dblock changed the title Clarified use of host and use path.combine for paths. Clarified use of host, use path.combine for paths and lodash.defaults for config. Expose config. Feb 6, 2017

@dblock

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 6, 2017

Appreciate a careful review/merge from @mreinstein or @tejashah88.

@tejashah88
Copy link
Member

left a comment

Few suggestions but otherwise looks good.

server_root: ''
public_html: 'public_html',
server_dir: 'server',
server_root: '.',

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 6, 2017

Member

I would recommend not defaulting server_root to ., rather __dirname would be safer. See http://stackoverflow.com/a/18283508 for more info.

This comment has been minimized.

Copy link
@dblock

dblock Feb 6, 2017

Author Collaborator

While safer, this would change the current default. Opened #73.

index.js Outdated
self.httpsInstance = httpsServer.listen(config.httpsPort, config.host);
self.log("listening on https://" + config.host + ":" + config.httpsPort);
self.httpsInstance = httpsServer.listen(self.config.httpsPort, self.config.host);
self.log("listening on https:// " + self.config.host + ":" + self.config.httpsPort);

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 6, 2017

Member

Might want to remove the space from "https://" so that if anyone is copying the exact url, it shouldn't be the following: "https:// 127.0.0.1:6000"

This comment has been minimized.

Copy link
@dblock

dblock Feb 6, 2017

Author Collaborator

Thanks for spotting this, typo fixed.

index.js Outdated
self.instance = self.express.listen(config.port, config.host);
self.log("listening on http://" + config.host + ":" + config.port);
self.instance = self.express.listen(self.config.port, self.config.host);
self.log("listening on http:// " + self.config.host + ":" + self.config.port);

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 6, 2017

Member

Same as above.

This comment has been minimized.

Copy link
@dblock

dblock Feb 6, 2017

Author Collaborator

Thanks for spotting this, typo fixed.

.expect(404);
});

it("defaults host to undefined, all IPs", function() {

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 6, 2017

Member

Technically it binds to any available IP, not all IPs.

This comment has been minimized.

Copy link
@dblock

dblock Feb 6, 2017

Author Collaborator

Updated.

@dblock dblock force-pushed the dblock:test-server-config branch 2 times, most recently from c7b4954 to 6134a86 Feb 6, 2017

@dblock dblock force-pushed the dblock:test-server-config branch from 6134a86 to c770c1d Feb 6, 2017

@dblock dblock merged commit e1097df into alexa-js:master Feb 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 95.513%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.