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

fix: Check port usage correctly (fix #6519) #6523

Closed
wants to merge 4 commits into from
Closed

fix: Check port usage correctly (fix #6519) #6523

wants to merge 4 commits into from

Conversation

dohooo
Copy link

@dohooo dohooo commented Jan 16, 2022

Description

fixed: #6519

I hope there is an accurate notification when a port is occupied, as this can take a lot of time for most novices to debug. He may not be able to tell if it's a problem with his business code or a problem with the project configuration.

Additional context

Should I continue to check localhost after checking for 127.0.0.1? Because we probably don't know if the user's 127.0.0.1 domain name is localhost either, although most of them are.


What is the purpose of this pull request?

  • Other
  • Bug fix
  • New Feature
  • Documentation update

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.

@dohooo
Copy link
Author

dohooo commented Jan 16, 2022

I added the corresponding tests.

@dohooo
Copy link
Author

dohooo commented Jan 16, 2022

@Niputi thx

@Niputi Niputi added p2-nice-to-have Not breaking anything but nice to have (priority) enhancement New feature or request labels Jan 16, 2022
Copy link
Contributor

@Niputi Niputi left a comment

Choose a reason for hiding this comment

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

please move the test to the playground folder which contains the tests

@dohooo
Copy link
Author

dohooo commented Jan 16, 2022

please move the test to the playground folder which contains the tests

ok

@dohooo dohooo changed the title feat: Check port usage correctly feat: Check port usage correctly (fix #6519) Jan 16, 2022
@dohooo dohooo changed the title feat: Check port usage correctly (fix #6519) fix: Check port usage correctly (fix #6519) Jan 16, 2022
@dohooo
Copy link
Author

dohooo commented Jan 16, 2022

I've already moved. Thanks for the tip. @Niputi

@dohooo dohooo requested a review from Niputi January 17, 2022 03:58
@Niputi
Copy link
Contributor

Niputi commented Jan 17, 2022

the test is failing

@dohooo
Copy link
Author

dohooo commented Jan 17, 2022

the test is failing

I see it looks like the dependency install failed how do I restart it?

@dohooo
Copy link
Author

dohooo commented Jan 17, 2022

the test is failing

Test was passed.

@bluwy
Copy link
Member

bluwy commented Jan 23, 2022

@dohooo I'm a bit confused. Port checking and using incremented ports are already available for dev and preview commands. It looks like the core issue is that Vite fails to detect port 5000 being occupied on macos, but I don't see how this change fixes it, can you elaborate on your changes here?

@dohooo
Copy link
Author

dohooo commented Jan 23, 2022

@dohooo I'm a bit confused. Port checking and using incremented ports are already available for dev and preview commands. It looks like the core issue is that Vite fails to detect port 5000 being occupied on macos, but I don't see how this change fixes it, can you elaborate on your changes here?

The default host of vite dev server is 127.0.0.1. When I start the same port server on localhost, It can't detect occupancy. So I'll check the port on localhost again when host is 127.0.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The preview mode doesn't correctly inform the user about port occupied.
3 participants