search

airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 212 forks source link

Add option to configure host #66

Closed magicmark closed 7 years ago

magicmark commented 7 years ago

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.

magicmark commented 7 years ago

@ljharb yep!

Express uses http.Server.listen() (https://expressjs.com/en/api.html#app.listen) Which defaults to 0.0.0.0 (https://nodejs.org/api/http.html#http_server_listen_port_hostname_backlog_callback)

ljharb commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

magicmark commented 7 years ago

@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 commented 7 years ago

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

@ljharb that was me, my bad!