Skip to content
This repository was archived by the owner on Oct 9, 2023. It is now read-only.

Add option to configure host - #66

Merged
goatslacker merged 1 commit into
airbnb:masterfrom
magicmark:add_host_option
Jun 16, 2017
Merged

Add option to configure host#66
goatslacker merged 1 commit into
airbnb:masterfrom
magicmark:add_host_option

Conversation

@magicmark

Copy link
Copy Markdown
Contributor

We'd like to be able to configure the host that express binds to.
This new config option passes it along, and defaults to 0.0.0.0.

@ljharb ljharb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is 0.0.0.0 the current default?

@magicmark

Copy link
Copy Markdown
Contributor Author

@ljharb

ljharb commented May 17, 2017

Copy link
Copy Markdown
Collaborator

Instead of hardcoding the default, maybe it'd be better to have a default of null, and use the 2-arg form when a truthy host isn't provided?

@magicmark

Copy link
Copy Markdown
Contributor Author

@ljharb Yeah sounds better, I'll do that.

Couple of things I wanted to ask you:

  1. I wasn't going to typecheck config.host - logic being that http.server.listen will do its own typechecking, and an invalid host value error will bubble up for us. Does that sound reasonable? Or should we be going our own typechecking here?
  2. In adding a test that the server doesn't blow up when specifying a host, I ran into an issue whereby I can't start a new instance of hypernova, because there's one already running from the previous test. Since the listen() and close() stuff is async, it requires a bit of refactoring to get into it to properly pass the done() callback (possibly exposing the 'server' attribute', or just adding a 'close' method to hypernova or something.) I can do this as a separate branch - any thoughts here?

Thanks!

@ljharb

ljharb commented May 17, 2017

Copy link
Copy Markdown
Collaborator

I prefer doing our own typechecking if possible.

If there's an issue with async tests, a separate PR to fix it, merged first, would be ideal.

@ljharb

ljharb commented Jun 15, 2017

Copy link
Copy Markdown
Collaborator

@magicmark would you mind rebasing on the command line, instead of clicking "update branch" on the github ui?

@magicmark

Copy link
Copy Markdown
Contributor Author

@ljharb sure. I haven't touched this branch in a while though.

(waiting to finish another branch to refactor the integration test so I can write a test for this change)

@goatslacker

Copy link
Copy Markdown
Collaborator

would you mind rebasing on the command line, instead of clicking "update branch" on the github ui?

@ljharb that was me, my bad!

@goatslacker
goatslacker merged commit e2956b3 into airbnb:master Jun 16, 2017
@magicmark
magicmark deleted the add_host_option branch July 21, 2017 21:47
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.

3 participants