Skip to content

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

Closed
wants to merge 12 commits into from

Conversation

knagaitsev
Copy link
Collaborator

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

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's listen method.

  • Firstly, the CLI will do significantly less configuration/error handling, as the API will do it instead
  • API users can find more use in start method, because it does not make sense that they have to provide host and port via listen(port, hostname, fn), when they probably already pass those into new Server(options) as dev server options.
  • API users can make use of socket option and the automatic free port assignment via start, or opt out of those options via listen.

Breaking Changes

Likely not, need to add tests to be sure though

Additional Info

@codecov
Copy link

codecov bot commented Jun 14, 2019

Codecov Report

Merging #2028 into master will increase coverage by 0.08%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/Server.js 92.55% <93.33%> (-0.18%) ⬇️
lib/utils/status.js 95% <0%> (+5%) ⬆️
lib/utils/findPort.js 94.44% <0%> (+5.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 661f6a2...93477fc. Read the comment docs.

throw new Error('This socket is already used');
});
}
});
Copy link
Member

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

Copy link
Collaborator Author

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?

@knagaitsev knagaitsev changed the title feat(server): new start method feat/refactor(server): move CLI server start into API listen method Jun 15, 2019
@knagaitsev
Copy link
Collaborator Author

Tests that need to be added for this:

socket option:

  • confirm that it works on CLI and API
  • simulate chmod failure to ensure that handler works
  • simulate EADDRINUSE for server start, then consequent ECONNREFUSED to see if the server can be created after initial failure

port option:

  • should still work as string and number on API/CLI, if the string parses to a number
  • on API, port should now go to a free port if an undefined one is provided

Copy link
Member

@alexander-akait alexander-akait left a 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:

  1. Move socket check
  2. Move findPort

Anyway looks good, better handle this in two PRs because:

  1. Easy review
  2. Less chance of making a mistake
  3. Easy create changelog

Anyway good job

@knagaitsev
Copy link
Collaborator Author

Let's separate this PR in two:

  1. Move socket check
  2. Move findPort

Anyway looks good, better handle this in two PRs because:

  1. Easy review
  2. Less chance of making a mistake
  3. Easy create changelog

Anyway good job

Closing in favor of #2061

@knagaitsev knagaitsev closed this Jun 24, 2019
@knagaitsev knagaitsev added the gsoc Google Summer of Code label Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc Google Summer of Code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants