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

replace on-finished with node:stream's finished #6414

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bjohansebas
Copy link
Member

This is part of supporting HTTP/2 within Express and also using native APIs.

I'm not sure if this is a breaking change. All tests are passing, although errors could change, so I would say it might be a breaking change. What do you think?

@bjohansebas bjohansebas requested a review from a team March 25, 2025 02:18
@bmuenzenmeyer
Copy link

I'm not sure if this is a breaking change. All tests are passing, although errors could change, so I would say it might be a breaking change. What do you think?

My unsolicited opinion / logic is as follows:

if the only thing someone does is upgrade express, and their CI breaks, it's a major. The error is part of the public API.

@bjohansebas
Copy link
Member Author

if the only thing someone does is upgrade express, and their CI breaks, it's a major. The error is part of the public API.

But the middleware could depend on that error, because there could be middleware that depends on it—not the ones maintained by Express, but surely someone could be relying on it.

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.

3 participants