Skip to content

Conversation

@patak-dev
Copy link
Member

Description

Fix #3355

After #2977, we lost the hostname defaulting to localhost for '127.0.0.1' and '0.0.0.0'

if (hostname === '0.0.0.0') hostname = 'localhost'

This PR adds that logic back, also including host === undefined and '::' that should be treated equally to '0.0.0.0'

The reported URL in the console also replaces '127.0.0.1' by localhost now.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label May 12, 2021
@patak-dev patak-dev requested a review from antfu May 12, 2021 10:25
antfu
antfu previously approved these changes May 12, 2021
nihalgonsalves
nihalgonsalves previously approved these changes May 12, 2021
return Array.from(new Set(arr))
}

export function defaultHostname(host: string | undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (dx, readability): Isn't it more like a normalize or fallback?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is hard to name, it isnt a normalization or fallback either. More like useLocalhostIfPossible

Copy link
Member

@Shinigami92 Shinigami92 May 12, 2021

Choose a reason for hiding this comment

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

Ah I see, I trapped and thought it would be the other way around

useLocalhostIfPossible is ugly, but definitely more expressive about what happens 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, in #3383 this function is gone so we can discuss how to name other things there :)

@patak-dev patak-dev dismissed stale reviews from nihalgonsalves and antfu via 144f405 May 12, 2021 11:18
@patak-dev patak-dev requested a review from nihalgonsalves May 12, 2021 11:20
@patak-dev patak-dev marked this pull request as draft May 12, 2021 11:31
? 'Local: '
: 'Network: '
const host = detail.address
const host = detail.address.replace('127.0.0.1', 'localhost')
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use defaultHostname here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is going to be always 'localhost' when calling it.

@dominikg
Copy link
Contributor

hmm, would this still allow to explicitly pass --host 0.0.0.0 to allow access via all interfaces? and show correctly on console?

@patak-dev
Copy link
Member Author

hmm, would this still allow to explicitly pass --host 0.0.0.0 to allow access via all interfaces? and show correctly on console?

I'll close this PR if we merge this alternative #3383

About --host 0.0.0.0, it will work as in 2.2, the console will show all available addresses, and only the 127.0.0.1 will show as localhost in the console and when doing --open.

@antfu antfu closed this in #3383 May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p4-important Violate documented behavior or significantly improves performance (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If I set server.host as true and server.open it doesn't work

5 participants