Thank you for the pull request! What do you think about checking for app.get('env') === 'production' instead of the port? I'm not sure if checking for the port is enough since the app often runs behind an NginX proxy on a different port.

@daffl I questioned that, too. There are myriads of configurations. Even in dev environments, it isn't that uncommon to have a dockerized environment behind an nginx running HTTPS (https://github.com/jwilder/nginx-proxy is a good one).

Perhaps a better way to handle this would be to not add the port at all, and assume that it is included in the host config if it is non-standard? WDYT?

The reason I was suggesting based on the environment is that I would like to cover the following two cases without having to configure anything:

1. When following the guide (this usually means it's in development and runs on a non-standard port)
2. In a production environment where the app is running on a standard port either behind a proxy or directly

For everything else it's always possible to customize the port and hostname oauth settings.

Okay, let's game this out:

1. HTTP, env=development, port 80: url = http://localhost ✅
2. HTTP, env=development, port 8000: url = http://localhost:8000 ✅
3. HTTPS, env=development, port 443: url = http://localhost:443 ❌
4. HTTPS, env=development, port 8443: url = http://localhost:8443 ❌
5. HTTP, env=production, port 80: url = https://domain:80 ❌
6. HTTP, env=production, port 8000: url = https://domain:8000 ❌
7. HTTPS, env=production, port 443: url = https://domain ✅
8. HTTPS, env=production, port 8443: url = https://domain:8443 ✅

Do you care about 3-6 being wrong by default, thus needing special configuration? If so, then we need to change the assumption of https in production/http everywhere else.

I think that's ok especially since all it takes is a configuration setting. 5. and 6. should definitely be discouraged and 3. and 4. is not covered by the standard environment that's generated by the Feathers CLI.

Does that make sense? Let me know if you are interested in updating the PR. Definitely agree that this should be improved.

@daffl since you don't care about 3-6, nothing needs to be changed on this PR. It will fulfill the behaviors listed in 1-2 and 7-8 by default. Am I missing something?

The idea is to make sure the following are true (based on this PR and on some previous issues/discussions) :

1. In production the port is never added since most likely people are using a proxy (missing in this PR)
2. Regardless of environment, ports http/80 and https/443 should not be appended (PR does that) (which also helps if node is getting direct traffic but using default ports)

For everything else, user's would configure a custom host, but these should cover most common cases.

I think your PR just needs the following change:

-if ((protocol === 'https' && port !== '443') || (protocol === 'http' && port !== '80'))
+if (env !== 'production' && ((protocol === 'https' && port !== '443') || (protocol === 'http' && port !== '80')))

and ideally tests to go over the different permutations to we avoid future regressions.

On a related, but slightly different topic, wondering how often Feathers checks for env === 'production'. I would suggest perhaps reversing that to env !== 'development' since there could be multiple production like environments (staging in particular is something I often use).

Thanks for that clarification and suggestion. This version should accomplish all our goals and is much easier to read to boot.

This looks good to me, just will need to see what @daffl thinks about the use of 'development' – it depends a bit on what Feathers uses elsewhere for consistency. Though it looks like it's using a bit of both. Wondering, for example, if we should do 'development' || 'test'..

Oh you're right, I didn't pay attention, sorry about that. Good to go then, thank you!