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

Propagate socket error event on res.destroy #449

Closed
mikicho opened this issue Sep 27, 2023 · 3 comments · Fixed by #597
Closed

Propagate socket error event on res.destroy #449

mikicho opened this issue Sep 27, 2023 · 3 comments · Fixed by #597
Assignees

Comments

@mikicho
Copy link
Contributor

mikicho commented Sep 27, 2023

According to docs, response.destroy(err) emits error event.:

If error is provided, an 'error' event is emitted on the socket and error is passed as an argument to any listeners on the event.

https://github.com/nodejs/node/blob/main/lib/internal/streams/destroy.js#L136


Nock handles this by overriding the Socket class and propagate the event to the request.

@kettanaito
I tried to open a PR for this but couldn't understand how to do this, but I believe I'll be able to with your direction.

@mikicho mikicho changed the title propagate socket error event on res.destroy Propagate socket error event on res.destroy Sep 27, 2023
@mikicho
Copy link
Contributor Author

mikicho commented Oct 5, 2023

Update: the response behavior isn't identical to the original response. IncomingMessage and PassThrough act differently on destroy
image

@mikicho
Copy link
Contributor Author

mikicho commented Oct 5, 2023

@kettanaito Why do we skip properties if they have the same name? The implementation may be still different
If I delete these lines my test passes because it calls to the correct implementation.

@kettanaito
Copy link
Member

Released: v0.32.1 🎉

This has been released in v0.32.1!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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 a pull request may close this issue.

2 participants