Michael/fix 2863#2867
Conversation
gr2m
left a comment
There was a problem hiding this comment.
Up to you wether to backport or not, but from my experience it will cause friction for v14 that you don't expect. When in doubt, I'd encourage folks to upgrade instead.
|
🎉 This PR is included in version 15.0.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
It would be much appreciated if this would be backported to 14. It is actually a bad experience if we update to a behavior and then have to revert that in the next major again to the former (more intuitive) one. |
|
@BridgeAR I'm concerned that this might introduce a breaking change, which could lead to user complaints about the different behavior (even if it's a bug). Nevertheless, I'm ok to attempt a backport and monitor for any issues. Feel free to open a PR. |
|
@mikicho I just looked at it for a bit but there are some conflicts in places that I can't determine easily what is right or wrong and I can't find the code change that removed this between 13 and 14. Could you open the backport yourself? I believe that is much easier for you knowing the code base :) I guess it could be tested by us and others who were facing the issues with the delay to see if the backport would cause any other issues before actually landing the PR. |
|
@BridgeAR Thanks for taking a look into it. I am currently focusing on different things in Nock. |
This fixes a regression where `delayBody(N)` no longer makes a body look
slow to the caller. Read timeouts on the response stream are expected to
fire when N exceeds them, but in v14 the @mswjs/interceptors-based
pipeline drains the response body before delivering the response event.
Pausing the body source (the v13 mechanism) ended up delaying the
*whole* response — response, data, and end events all moved to
`+delayBodyInMs`, indistinguishable from a slow connection.
Two coordinated pieces fix it:
1. `lib/playback_interceptor.js`: drop the pause/resume mechanism and
gate only the end-of-response push (`response.push(null)`) on
`delayBodyInMs`. Schedule that gate inside `respond()` so
`delay({ head, body })` continues to compound — head defers the
response event, body then defers the end signal by an additional
`body` ms. Reply callbacks like `replyWithFile` hand back an
explicitly-paused `fs.createReadStream`, so call
`bodyAsStream.resume()` to ensure the body flows.
2. `tests/got/test_delay.js`: the two tests that asserted timing on the
first `data` event are updated to assert on the `end` event,
matching the new (and intended) semantics.
Refs: #2867
Fixes: #2863
Resolved #2863 in the beta branch.
I'm wondering if we should backport it to v14, because it's a somewhat breaking change (but probably as desired). WDYT?