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

feat(ClientRequest): await response listeners before emitting the "end" response event #591

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

kettanaito
Copy link
Member

Changes

  • ClientRequest: Awaits all the response event listeners before finishing the response (emitting its end event). This affects both mocked and original responses (the approach is different for each).

@kettanaito kettanaito requested a review from mikicho July 3, 2024 13:23
@@ -419,7 +434,7 @@ export class NodeClientRequest extends ClientRequest {
const firstClone = cloneIncomingMessage(response)
const secondClone = cloneIncomingMessage(response)

this.emit('response-internal', secondClone)
this.emit('response-internal', secondClone, firstClone)
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to expose the original IncomingMessage of the original response if we wish to delay its end event emission. This modifies the listener call signature of the response-internal event.

@@ -643,4 +658,23 @@ export class NodeClientRequest extends ClientRequest {
// @ts-ignore "agent" is a private property.
this.agent?.destroy?.()
}

private deferResponseEndUntil(
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe premature but I'm creating a private method to help us reuse the same deferring logic for both mocked and original responses (the target IncomingMessage is different in case of both).

Copy link
Member Author

Choose a reason for hiding this comment

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

I will likely repurpose this for the Socket interceptor. It would have its own implementation of awaiting the response listeners.

@@ -69,6 +69,7 @@ beforeAll(async () => {
})

afterEach(() => {
interceptor.removeAllListeners('response')
Copy link
Member Author

Choose a reason for hiding this comment

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

Incredibly flaky test poorly written. Shame on me.

I'm resetting the response listener because they apparently leak between the tests through the shared interceptor instance.

I've been scoping request/response listeners within individual tests for some time to prevent this from happening but this is a really old test.

@kettanaito kettanaito changed the title feat(ClientRequest): await response events before response end feat(ClientRequest): defer emitting response "end" event until response listeners are awaited Jul 3, 2024
@kettanaito kettanaito changed the title feat(ClientRequest): defer emitting response "end" event until response listeners are awaited feat(ClientRequest): await response listeners before emitting the "end" response event Jul 3, 2024
@kettanaito kettanaito merged commit f68e0b6 into main Jul 3, 2024
2 checks passed
@kettanaito kettanaito deleted the feat/await-response-events branch July 3, 2024 14:11
@kettanaito
Copy link
Member Author

Released: v0.31.0 🎉

This has been released in v0.31.0!

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 this pull request may close these issues.

Await all "response" listeners to finish
1 participant