fix: Ensure Duplex streams are returned unwrapped in stream operators

[KunalKumar-1]

Updated the implementation of the readable.compose() method to ensure that it correctly returns a Duplex stream when applicable, avoiding unintended conversion to Readable streams.
fixes: #55203

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[avivkeller]

Please add a test

[KunalKumar-1]

@redyetidev test added

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63127/

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.35%. Comparing base (51d8146) to head (90ac00d).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/stream.js	66.66%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55394      +/-   ##
==========================================
- Coverage   88.41%   88.35%   -0.07%     
==========================================
  Files         652      652              
  Lines      186878   186897      +19     
  Branches    36061    36024      -37     
==========================================
- Hits       165226   165126     -100     
- Misses      14902    15017     +115     
- Partials     6750     6754       +4     

Files with missing lines	Coverage Δ
lib/stream.js	94.37% <66.66%> (-5.63%)	⬇️

... and 32 files with indirect coverage changes

[KunalKumar-1]

@mcollina why the test cases are failing ?

[ronag]

I'm on the fence here. The returned stream should not be written to so semantically it makes more sense to return an explicit readable. Though there is some performance costs associated with this.

@mcollina @lpinca wdyt?

[KunalKumar-1]

@ronag I see your point regarding the stream semantics. If performance impact is minimal, then returning an explicit Readable does make sense to clarify the intended use.
Could you clarify the exact changes you’d like to see here?

[ronag]

Could you clarify the exact changes you’d like to see here?

Well if consider my concern. Then we should just leave this as is? We might ned update docs/typings to clarify that if the first param (this) to compose is not writable then we return a readable instead of duplex, ditto with if the last stream is not readable, we return a writable.

[ronag]

Alternatively returning a new Duplex with writable: false

[KunalKumar-1]

Alternatively returning a new Duplex with writable: false

After considering both options, would it be preferable to return a new Duplex with writable: false, or should we lean towards your suggestion of simply returning a Readable or Writable stream based on the first and last streams in the composition?

I’m inclined to go with whichever approach aligns best with the project’s consistency and performance considerations !

[ronag]

I'm happy with the Duplex approach. Give it a day or two for others to also voice their opinions before you spend more time on this. Otherwise, you might risk doing more work.

[KunalKumar-1]

I'm happy with the Duplex approach. Give it a day or two for others to also voice their opinions before you spend more time on this. Otherwise, you might risk doing more work.

Yeah , sure It sounds great.
I Will start working on it after a day or two ....
After hearing some options.

[Renegade334]

I'm on the fence here. The returned stream should not be written to so semantically it makes more sense to return an explicit readable. Though there is some performance costs associated with this.

readable.compose(x) should have the same behaviour as stream.compose(readable, x) imo (ie. return a read-only Duplex), given that the former is essentially just a wrapper for the latter. This also keeps things consistent if called as duplex.compose(x), which should also return a Duplex (read-only or read-write depending on the source stream).

After considering both options, would it be preferable to return a new Duplex with writable: false

It's worth mentioning that this is already what's returned by the internal compose() operator, so wouldn't need any further handling.

Indeed, if the desired behaviour of readable.compose() is that it just passes through the Duplex returned by the compose operator, then it wouldn't need to be included in streamReturningOperators at all; the operator could just be added straight onto Readable.prototype.