Review requested:

• @nodejs/streams

Because if the chunk from the source stream is not object while source stream is in object mode and destination stream is not in object mode, it should still work

I don't think it should be supported. It is most likely a programmer error.

I don't think it should be supported. It is most likely a programmer error.

I agree, but I am afraid this would break many user land packages.

We could mark it as semver-major and run CITGM. I think it should be fixed in the "broken" packages and I don't think there are many of them.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.96%. Comparing base (3bb1d28) to head (b1c5b00).
Report is 161 commits behind head on main.

@@           Coverage Diff           @@
##             main   #55270   +/-   ##
=======================================
  Coverage   87.95%   87.96%           
=======================================
  Files         656      656           
  Lines      188310   188315    +5     
  Branches    35963    35973   +10     
=======================================
+ Hits       165630   165645   +15     
+ Misses      15854    15848    -6     
+ Partials     6826     6822    -4     

I've been thinking about it, and I think we should more carefully consider whether we should throw directly in pipe method or not.

The reasons being:

• Readable.from will have objectMode: true by default which means the destination stream has to have objectMode: true, and we have a few tests to demonstrate that this would fail e.g.

  node/test/parallel/test-stream-pipeline.js

  Line 1357 in e79ae1b

  const w = new Writable({

  
  The Writable would require objectMode: true
• This could potentially break many user land code - previously I mentioned user land package, without CITGM I wouldn't have a confident answer, however I have spoken to a few friends and they have had a scan in their projects and found they would need to add objectMode: true in many places (keep in mind, this could also be a very biased result)

To wrap up, what I am trying to say is that we may wanna consider this breaking change more carefully as all the Readable.from will default to objectMode: true and we support passing non-object as data when objectMode is true.

cc. @nodejs/streams

@mcollina I think you misunderstand this PR. It basically fixes an uncatchable error. Doesn't break anything as far as I can tell.

Though I do think this PR is a little over engineered. It's enough to catch the error and forward it "as is" to destroy.

@mcollina at the moment this only catches the error thrown by dest.write() and call dest.destroy() with it, so I think it will not break anything. It only addresses #54945. That being said, I think the crash in #54945 is expected and "wanted". We make the process crash on programmer errors.

Thank you for the review! 🙏 I appreciate the feedback and am happy to remove this error. As @ronag pointed out, it seems like I may have over-engineered it. My initial intention was to help developers understand the error better, but looking back, it appears I might've complicated things more than I've fixed.

Another point for discussion is whether we should catch the error and call destroy on the dest. As @lpinca noted that this is a programmatic error and that a crash is expected.

Another point for discussion is whether we should catch the error and call destroy on the dest. As @lpinca noted that this is a programmatic error and that a crash is expected.

I disagree with @lpinca on this one and would prefer a catchable error. The exception guarantee is understandable and it's possible to isolate the error from other working parts of a service, i.e. better to have a http server return 500 on a broken path than stop responding on all paths.

The exception guarantee is understandable and it's possible to isolate the error from other working parts of a service, i.e. better to have a http server return 500 on a broken path than stop responding on all paths.

The error is already understandable (your suggestion is to call dest.destry() with the same error) and is not recoverable. If that error occurs, there is something wrong in the user code and not Node.js code.

I would also argue that if we want this behavior we should make writable.write() call writable.destroy() on invalid input instead of throwing an error.

@ronag this change of behavior would be semver-major anyway.

I'm still unsure about whether we should catch the error or proceed with a hard crash. Is there a similar example in Node.js core that we could use for comparison?

On the other hand, does those lines make sense?

diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js
index ac14b202b6..5225aa13f4 100644
--- a/lib/internal/streams/writable.js
+++ b/lib/internal/streams/writable.js
@@ -477,8 +477,9 @@ function _write(stream, chunk, encoding, cb) {
       chunk = Stream._uint8ArrayToBuffer(chunk);
       encoding = 'buffer';
     } else {
-      throw new ERR_INVALID_ARG_TYPE(
-        'chunk', ['string', 'Buffer', 'TypedArray', 'DataView'], chunk);
+      stream.destroy(new ERR_INVALID_ARG_TYPE(
+        'chunk', ['string', 'Buffer', 'TypedArray', 'DataView'], chunk));
+      return
     }
   }

or I should move what I've done in the catch block into dest.write function.

I think your PR was fine other than there is no need to convert it into another error.

This is the error I've got

That's the expected error.
With node 22.12.0 the command just hang without showing the error. Like:

$ jhipster --defaults
.
.
.
identical src/test/java/com/mycompany/myapp/service/UserServiceIT.java
identical .lintstagedrc.cjs
$ echo $?
0

do you mind explaining a bit more on why you think this is related to the pipeline?

Looks related to pipeline transforms like:

      Duplex.from(async function* (files: AsyncGenerator<MemFsEditorFile>) {
        for await (const file of files) {
          if (isFilePending(file)) {
            yield file;
          }
        }
      }),

And how do I test with node v22.4 vs v22.x

I use n to switch between node versions.

n 22.12.0
jhipster --defaults

n 22.4.0
jhipster --defaults

would also be good if you could also pin point me to the code in the generator-jhipster project that you think might be the issue. Cheers.

It's not a single place, yeoman/generator#1577, SBoudrias/mem-fs-editor#282, SBoudrias/mem-fs#40.
With those changes the error is correctly shown in node 22.12.0.

Hi @aduh95 I see you've helped with the CITGM result here - #55270 (comment) 🙏

I have to admit, I'm not entirely sure how to interpret the results. Is there any documentation that explains how to understand the CITGM results? For this PR, are there any user-land packages I should examine more closely?

Landed in fd8de67