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

http server api change #3233

Merged
merged 7 commits into from Nov 6, 2019

Conversation

@lperlaki
Copy link
Contributor

lperlaki commented Oct 29, 2019

fixes #3228

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 29, 2019

CLA assistant check
All committers have signed the CLA.

std/http/server.ts Outdated Show resolved Hide resolved
const parts = this.listener.addr().address.split(':')
const port = parts.pop();
let hostname = parts.join(':');
if (hostname === '0.0.0.0' || hostname === '127.0.0.1' || hostname === '[::]' || hostname === '[::1]')

This comment has been minimized.

Copy link
@IllusionPerdu

IllusionPerdu Oct 29, 2019

0.0.0.0 is all IPv4 addresses => 127.0.0.1 is one of possible. If a host has two IP addresses, 192.168.1.1 and 10.1.2.1, and a server running on the host listens on 0.0.0.0, it will be reachable at both of those IPs.
[::] is all IPv6 addresses => idem for ipv6

I think it's important not to indicate localhost for this addresses!

const port = parts.pop();
let hostname = parts.join(':');
if (hostname === '0.0.0.0' || hostname === '127.0.0.1' || hostname === '[::]' || hostname === '[::1]')
hostname = 'localhost'

This comment has been minimized.

Copy link
@ry

ry Oct 29, 2019

Collaborator

localhost does not always map to 127.0.0.1 or ::1 ... this condition should be removed and the IP used

This comment has been minimized.

Copy link
@lperlaki

lperlaki Oct 29, 2019

Author Contributor

Ok, I removed the condition.

  1. s.url to return "http://localhost:8000/"

just though you wanted it to say localhost

lperlaki and others added 4 commits Oct 29, 2019
@ry
ry approved these changes Nov 6, 2019
Copy link
Collaborator

ry left a comment

LGTM - thanks!

@ry ry merged commit ccc9f1a into denoland:master Nov 6, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@lperlaki lperlaki deleted the lperlaki:better_http_server_api branch Nov 6, 2019
@sholladay

This comment has been minimized.

Copy link

sholladay commented Nov 10, 2019

@ry so s.url is not going to be a thing?

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Nov 10, 2019

@sholladay nah it was just a random thought of mine - leaving it out is simpler

@sholladay

This comment has been minimized.

Copy link

sholladay commented Nov 10, 2019

I'm planning to add server.url to Pogo so that people can easily log the URL or script their browser to open it. I'm fine with doing this in userland, but would have been nice for core to provide it. I would certainly use it, FWIW. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.