-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat/refactor(server): move CLI server start into API listen method #2028
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2028 +/- ##
==========================================
+ Coverage 92.86% 92.95% +0.08%
==========================================
Files 29 29
Lines 1149 1192 +43
Branches 327 331 +4
==========================================
+ Hits 1067 1108 +41
- Misses 78 80 +2
Partials 4 4
Continue to review full report at Codecov.
|
throw new Error('This socket is already used'); | ||
}); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can solve this without new method, it is very inconsistently with node api for servers, we can move this code without new method, just listen error
event and handle this inside server class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean handle it inside the listen(port, hostname, fn)
method, rather than making a new method?
Tests that need to be added for this:
|
…k-dev-server into cli-server-start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separate this PR in two:
- Move socket check
- Move findPort
Anyway looks good, better handle this in two PRs because:
- Easy review
- Less chance of making a mistake
- Easy create changelog
Anyway good job
Closing in favor of #2061 |
For Bugs and Features; did you add new tests?
Not yet, would like feedback on the idea first.
Motivation / Use-Case
I have added a
start
method in the API, which the CLI calls, rather than calling the API'slisten
method.start
method, because it does not make sense that they have to providehost
andport
vialisten(port, hostname, fn)
, when they probably already pass those intonew Server(options)
as dev server options.socket
option and the automatic free port assignment viastart
, or opt out of those options vialisten
.Breaking Changes
Likely not, need to add tests to be sure though
Additional Info