-
Notifications
You must be signed in to change notification settings - Fork 26
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
Expose express app #27
Conversation
"prepublishOnly": "npm run build", | ||
"build": "tsc --project tsconfig.json" | ||
"build": "tsc --project tsconfig.json", | ||
"prepare": "npm run build" |
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.
This has also changed. This means the package should build on local install (only) automatically, rather than just publish.
@@ -114,13 +114,9 @@ export class AppService extends EventEmitter { | |||
this.server = serverApp.listen(port, hostname, backlog, callback); | |||
return; | |||
} | |||
return new Promise((resolve, reject) => { | |||
this.server = serverApp.listen(port, hostname, backlog, (err: Error|null) => { |
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.
The latest types for express removed the err
parameter.
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.
That comes from Node's server.listen()
.
Looks like nowadays you're either waiting from an listening
or error
event.
server.on('listen', callback)
is the same as the callback parameter.
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've tweaked it so we listen for error. It was a bit tedious because serverApp might either be a Server or an Application, but I think we're good now.
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.
Looks good to me.
Lots of bridges add their own paths, so let's expose the express app so they don't have to hack around it.