Skip to content

Conversation

@yowainwright
Copy link
Member

Description

This PR adds a minimal fix + tests for #1913.
Thanks @nuintun for their observation and solutions.

Checklist

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (1c642f4) to head (07c0c49).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1914   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files           9        9           
  Lines        2081     2084    +3     
=======================================
+ Hits         2079     2082    +3     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yowainwright yowainwright requested a review from Copilot October 27, 2025 03:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where setting res.body to a new stream would incorrectly destroy the original stream, breaking middleware patterns that wrap streams (like compression middleware). The fix prevents destroying the original stream when the new value is also a stream, while still cleaning up streams when replaced by non-stream values.

Key Changes:

  • Modified stream cleanup logic to only destroy the original stream when replaced by a non-stream value
  • Updated test expectations to reflect that streams should not be destroyed when replaced by other streams
  • Added a new test case demonstrating the wrapping stream middleware pattern

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/response.js Added conditional check to prevent destroying original stream when new value is also a stream
tests/response/body.test.js Updated test expectations and added new test case for wrapping stream middleware pattern

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yowainwright yowainwright marked this pull request as ready for review October 27, 2025 03:50
@yowainwright yowainwright requested a review from fengmk2 October 27, 2025 03:51
@fengmk2 fengmk2 changed the title fix: adds minimal fix for ISSUE-1913 fix: only original value destroy if the new value is not a stream Oct 27, 2025
@fengmk2 fengmk2 merged commit 863c211 into master Oct 27, 2025
6 checks passed
@fengmk2 fengmk2 deleted the ISSUE_1913 branch October 27, 2025 04:23
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