Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Call callback once on listen error #3216

Merged
merged 1 commit into from
May 17, 2024

Conversation

wesleytodd
Copy link
Member

This is a redo of #2623. Probably didn't need a new PR, but I took a new direction by using the once module, and only binding to the error event. I think this is slightly more elegant than the previous PR. As discussed, this is a breaking change, so can only land in a 5.x branch.

@dougwilson
Copy link
Contributor

For some reason the test is not happy on Node.js 0.10. That may not actually be a problem with Express 5.0 target, just wanted to call out why the CI failed on this one (for now).

@dougwilson dougwilson added the 5.x label Feb 21, 2017
@dougwilson dougwilson self-assigned this Feb 21, 2017
@wesleytodd
Copy link
Member Author

Oh, this is a change since 0.12, but if the server fails to start, calling close will error. The test introduces this case. In this case I can just not call close I think. Updated.

@wesleytodd
Copy link
Member Author

Anything blocking this from merging?

@RobinTail
Copy link
Contributor

Do you have enough permissions to merge it now, @wesleytodd ?

@wesleytodd
Copy link
Member Author

I had enough permissions back then I think, I was just more making sure we had agreement 😄. I will be re-visiting these pr's over the week and will land them as we are sure they are good.

@wesleytodd wesleytodd force-pushed the server-listen-error branch from cf5ce72 to 103e711 Compare May 14, 2024 02:19
@wesleytodd
Copy link
Member Author

I updated this, but I would like a second look from @expressjs/express-tc or others to double check since this is soooooo old. If no one has opposed this change or given feedback I will merge it later this week.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants