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

[feat] Use dynamic port if specified port is already in use #2441

Conversation

baseballyama
Copy link
Member

This is related to #948

Summary of the PR

At least on Windows, when I execute npm run dev like the above issue's reproduction steps, I got the below error if 3000 port is already in use.

Error: listen EADDRINUSE: address already in use 127.0.0.1:3000

I think this is correct behavior.
But on the other hand, webpack-dev-server has the feature that dev server finds free port if a default port is already in use.
https://github.com/webpack/webpack-dev-server/pull/685/files

So I implemented almost the same feature.
If a user specifies a specific port, dev server will check only the port.
But if not, dev server searches usable port from 3000.

Questions

  • Is there any issue with this PR?
  • Should I add some tests? (But honestly, I don't know how to write tests for this PR...)
  • Should I add/edit some documents?

Thank you!!


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

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Sep 16, 2021

🦋 Changeset detected

Latest commit: 76950f2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Conduitry
Copy link
Member

I'd want this to be run by Rich first, because IIRC he had opinions about apps automatically picking different ports rather than noisily failing and forcing you to say what port you wanted to use.

I also don't know anything about this dependency you're adding, and adding dependencies is something that should always be looked at very carefully. I see that portfinder has mkdirp as a (prod) dependencies, which is already setting off alarms for me. Why on earth would a library designed to find available ports need to create directories? When I install portfinder in a new directory, I get a node_modules of 6 megabytes, which is absolutely ridiculous.

@bluwy
Copy link
Member

bluwy commented Sep 16, 2021

We should be able to easily craft our own port finding logic too with the current port-authority package. Don't think we need the portfinder package either.

@baseballyama
Copy link
Member Author

@Conduitry

Thank you for telling the background.
So I felt that the existing implementation has intention.
So @bluwy gave me the alternative idea but should I spend more time on it?
Or should I close it at least for now?

adding dependencies is something that should always be looked at very carefully.

Agree with you. and sorry for that.
(Maybe it was a little bit casual...)

@benmccann
Copy link
Member

This functionality is already available in Vite with the strictPort: false option: https://vitejs.dev/config/#server-strictport

I think a better solution to this problem would be to merge #2232, so that the Vite server options take effect, which would remove a lot of confusion that people have about those options not doing anything today

@benmccann benmccann changed the title [feat] Use dinamic port if specified port is already in use [feat] Use dynamic port if specified port is already in use Sep 16, 2021
@benmccann benmccann closed this Sep 16, 2021
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.

4 participants