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 httpServer error handling. #103

Merged
merged 1 commit into from
Jun 18, 2019
Merged

Conversation

saiya
Copy link
Contributor

@saiya saiya commented Jun 18, 2019

Fixed to handler httpServer start up error.

For example, current marp-cli ignores EADDRINUSE error as below:

# Start some web server in port 8080
% marp --html --server .
[  INFO ] [Server mode] Start server listened at http://localhost:8080/ ...

# No error message.

Changes:

  • Properly handle 'error' event of node.js httpServer
  • Fixed "Unhandled promise rejection" problem on server startup error
  • Handle EADDRINUSE to show user friendly message

httpServer.on('error', (err) => {
httpServer.close()
if (err['code'] === 'EADDRINUSE') {
reject(new CLIError(err.message))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shows error message like this:

% ./marp-cli.js --html --server .
[ ERROR ] listen EADDRINUSE: address already in use :::8080

@@ -39,7 +39,20 @@ export class Server {

async start() {
this.setup()
new Promise<void>(res => this.server!.listen(this.port, res))
return new Promise<void>((resolve, reject) => {
Copy link
Contributor Author

@saiya saiya Jun 18, 2019

Choose a reason for hiding this comment

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

Fixed to return Promise instance.

Current code causes "Unhandled promise rejection" because this Promise was not handled.

@saiya saiya force-pushed the server_error_handling branch 5 times, most recently from b1bbcf5 to 83ba542 Compare June 18, 2019 09:39
@yhatt
Copy link
Member

yhatt commented Jun 18, 2019

Thanks for super nice improvement 😍

@yhatt yhatt merged commit dfbdd11 into marp-team:master Jun 18, 2019
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.

2 participants