Skip to content

feat(ext/http): Show that is also listening on localhost #28171

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

Merged
merged 11 commits into from
Apr 25, 2025

Conversation

WasixXD
Copy link
Contributor

@WasixXD WasixXD commented Feb 18, 2025

Closes #27722

This will show Listening on http://0.0.0.0:8000 (accessible via http://localhost:8000)

@CLAassistant
Copy link

CLAassistant commented Feb 18, 2025

CLA assistant check
All committers have signed the CLA.

@devsnek
Copy link
Member

devsnek commented Feb 19, 2025

This isn't equivalent at all. 0.0.0.0 is all interfaces, while localhost (127.0.0.1) is only one ip on one loopback interface. I guess, based on the linked issue, you want to make it more user friendly to people who don't understand this routing behavior. Maybe you can test for the socket address being 0.0.0.0 or :: and output an additional bit of text (while still being clear that its listening on 0.0.0.0)

@WasixXD
Copy link
Contributor Author

WasixXD commented Feb 19, 2025

Thanks for the insights, I'll redo it

@WasixXD
Copy link
Contributor Author

WasixXD commented Feb 20, 2025

This isn't equivalent at all. 0.0.0.0 is all interfaces, while localhost (127.0.0.1) is only one ip on one loopback interface. I guess, based on the linked issue, you want to make it more user friendly to people who don't understand this routing behavior. Maybe you can test for the socket address being 0.0.0.0 or :: and output an additional bit of text (while still being clear that its listening on 0.0.0.0)

With additional bit of text you mean like:

Listening on http://0.0.0.0:8000 (accessible via http://localhost:8000)

?

Or just showing Listening on http://localhost:8000 should be enough and I just correct the tests cases?

@devsnek
Copy link
Member

devsnek commented Feb 20, 2025

Listening on http://0.0.0.0:8000/ (accessible via http://localhost:8000/)

This seems like the ideal behavior to me, if you're willing to implement it.

@WasixXD WasixXD changed the title feat(ext/http): Show localhost instead of 0.0.0.0 feat(ext/http): Show that is also listening on localhost Feb 25, 2025
@WasixXD
Copy link
Contributor Author

WasixXD commented Mar 14, 2025

WDYT? @devsnek @dsherret

@dsherret dsherret requested a review from devsnek April 16, 2025 16:44
@dsherret dsherret added this to the 2.3.0 milestone Apr 16, 2025
? `(accessible via http://localhost:${addr.port})`
: "";

import.meta.log("info", `Listening on ${url} ${helper}`);
Copy link
Member

Choose a reason for hiding this comment

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

nit: the space between url and helper should be part of the helper string

import.meta.log("info", `Listening on ${scheme}${host}:${addr.port}/`);
const url = `${scheme}${host}:${addr.port}/`;
const helper = addr.hostname === "0.0.0.0" || addr.hostname === "::"
? `(accessible via http://localhost:${addr.port})`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? `(accessible via http://localhost:${addr.port})`
? `(accessible via ${scheme}://localhost:${addr.port})`

@WasixXD WasixXD requested a review from devsnek April 17, 2025 12:27
@bartlomieju bartlomieju self-assigned this Apr 22, 2025
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@bartlomieju bartlomieju enabled auto-merge (squash) April 25, 2025 08:00
@bartlomieju bartlomieju merged commit 9ed2fad into denoland:main Apr 25, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deno --init serve: http://0.0.0.0:8000 does not work with Safari
5 participants