Allows separating public host name from server IP address binding

[roblg]

I was playing around w/ react-server in cloud9's IDE (https://c9.io/) and ran into an issue where passing --host into the server listen (the default behavior of react-server-cli) did the wrong thing. I think the issue is due to cloud9's environment being containers running on a VM, where multiple public hostnames share the same public IP address, and the port that is presented to the container is not the actual physical port on the machine.

This PR separates the hostname given for loading resources like JS/CSS from the IP address that the server binds to, defaulting to binding to any IP address.

This fixes the issue for me (after I hand-modify the RequestToPort middleware so that it doesn't assume port 3000 😁) However, container networking isn't necessarily my forte, so if there are other/better ideas on how to fix the issue, I would love to hear them.

[gigabo]

What is this I don't even. :squint:

Cool inline default syntax. Shouldn't defaults go in defaultOptions.js, though?

[roblg]

Shouldn't defaults go in defaultOptions.js, though?

I... um. Yes, probably. Will do.

[gigabo]

If we call it bindIp internally to disambiguate, maybe the CLI option should be --bind-ip, too?

[roblg]

Can do.

[gigabo]

This makes a lot of sense. So generally we would want --host=... to be a domain name now?

The description in parseCliArgs.js is:

Hostname to start listening for react-server

Should probably update that.

... is this a breaking change? 🤔

[roblg]

So generally we would want --host=... to be a domain name now?

I was under the assumption that that is how it was being used already (if it's used at all?), since whatever you put there seems to end up in the URLs for static content when you're developing.

The description...

Will update to 'the public host name used to construct URLs for static content'?

... is this a breaking change? 🤔

I think it is, technically, though the broken case I can come up with seems vanishingly unlikely right now.

If my understanding is correct, the default behavior now will be to bind to any (or maybe "all" is more correct?) IP address on the machine were react-server is running, whereas previously, it would bind to whatever IP address the hostname resolved to. If anybody was relying on react-server being bound to a specific interface in production, I think that behavior will change. If foo.com resolves to 1.2.3.4 and bar.com resolves to 2.4.6.8, but both are hosted on the same machine, and both are running react-server, it seems like one of those two servers won't start up without passing --bind-ip to both of them.

[gigabo]

I'm suddenly confused. Wondering how react-server.io works a all with "host": "localhost". 😕

[aickin]

I thought (though maybe I'm misremembering?) that host was used to construct URLs to the static server. If bindIp is used instead, then won't the static URLs end up being https://0.0.0.0:3001/somestatic.bundle.js?

I might be totally confused about this, it's been a while.

[roblg]

I thought (though maybe I'm misremembering?) that host was used to construct URLs to the static server.

@aickin That's correct. --bind-ip shouldn't be used instead, it should be used in addition, and only then if you want that the binding behavior. I expect 99% of folks will just use --host exactly like they used to, and will be none the wiser. For weirdos like me using cloud IDEs in docker containers with a ton of different network interfaces with different IP addresses, they'll use --host like normal (to set the host from which static content is loaded), but also set --bind-ip to whatever their container's mapped IP address is.

The trigger for this change was that we're passing the value of --host into server.listen, which is incorrect in some cases.

[roblg]

I'm suddenly confused. Wondering how react-server.io works a all with "host": "localhost". 😕

@gigabo not sure, but trying to figure it out. My pet theory (with no hard evidence) is that the answer rhymes with "docker" or "nginx".

[aickin]

@aickin That's correct. --bind-ip shouldn't be used instead, it should be used in addition, and only then if you want that the binding behavior. I expect 99% of folks will just use --host exactly like they used to, and will be none the wiser.

I get that, I'm just confused because this PR seems to me like it deletes host as a parameter completely, but I think that might just be me misunderstanding how the code works these days. I'm grepping through to try and figure out where host is now used.

[gigabo]

where host is now used.

It's still used in packages/react-server-cli/src/run.js to build up static asset URLs.

options.outputUrl = jsUrl || `${httpsOptions ? "https" : "http"}://${host}:${jsPort}/`;

[aickin]

@gigabo Got it. That's what I was missing. And outputUrl is trickily passed through start.js to compileClient.js without being referenced in start.js.

Thanks for listening to the mumblings of a frequently confused man. ;)

[roblg]

And outputUrl is trickily passed through start.js to compileClient.js without being referenced in start.js.

@aickin Not even going to tell you how long I spent trying to understand that flow this morning. I saw the expected behavior (--host did the right thing), but couldn't figure out wth was going on. Even more confused because of the default syntax for outputUrl in compileClient.js.

[aickin]

@roblg best part is: it was probably my doing, and even I couldn't understand it any more!

<evil cackle, alternating with sobs>

[roblg]

The fog is clearing. Turns out the broken behavior being targeted by this PR is a regression introduced relatively recently by #665, which started passing the value of --host into server.listen.

@gigabo turns out that the "host": "localhost" in the react-server website doesn't work right now, as it was also broken by #665. It exhibits the same behavior I was seeing in cloud9, where no connections will be accepted because it's only listening to connections from localhost. This PR makes it work again with no additional change, since --bind-ip defaults to accepting connections from all interfaces ("0.0.0.0").

@SeverS after this PR merges, if you want to bind react-server to a particular IP address, you'll need to add --bind-ip in addition to --host. (You can pass a hostname to --bind-ip if you want, and node will do a DNS lookup automatically, e.g react-server --host foo.com --bind-ip foo.com)

For those following along at home, there's some additional context in this SO post about what happens to the hostname parameter to server.listen: http://stackoverflow.com/a/3173447/567229. Also, apparently I'm not the only one on the internet who was confused why it's called hostname, when it appears to actually relate to IP addresses. :)

[gigabo]

Whoa! Glad you tracked this down before the website horked!

Nice investigation, @roblg thanks for fixing this! 🤘

[roblg]

Is it safe to merge this at this point? Are we worried about the possibly-breaking part of these changes?

[gigabo]

Yeah, do it!