fix: use readable-stream

[ronag]

No description provided.

[ronag]

Could have impact when vendoring into node. Should probably use proper stream in that case... is there an easy fix for that? i.e. alias readable-stream with stream when required by core? @targos

[mcollina]

We should definitely not bundle readable-stream when preparing the code for core.

[ronag]

@targos

[targos]

What about this:

diff --git a/package.json b/package.json
index a02ae10..222a235 100644
--- a/package.json
+++ b/package.json
@@ -41,7 +41,8 @@
     "docs"
   ],
   "scripts": {
-    "build:node": "npx esbuild@0.14.38 index-fetch.js --bundle --platform=node --outfile=undici-fetch.js",
+    "build:node": "npx esbuild@0.15.15 index-fetch.js --bundle --external:readable-stream --platform=node --outfile=undici-fetch.js",
+    "postbuild:node": "sed -i 's/require(\"readable-stream\")/require(\"stream\")/g' undici-fetch.js",
     "prebuild:wasm": "docker build -t llhttp_wasm_builder -f build/Dockerfile .",
     "build:wasm": "node build/wasm.js --docker",
     "lint": "standard | snazzy",

[ronag]

Thank you! Applied.

[ronag]

@mcollina wdyt?

[mcollina]

CI seems really unhappy with this change.

[ronag]

@mcollina yea, assuming I managed to fix that, do you have any further objections?

[mcollina]

Not, it should have been like this from the beginning but we were stuck with a very old and legacy version of readable-stream.

[targos]

Isn't the fix simply to npm install readable-stream ?

[codecov-commenter]

Codecov Report

Base: 90.27% // Head: 94.85% // Increases project coverage by +4.58% 🎉

Coverage data is based on head (213cbfe) compared to base (93f150f).
Patch coverage: 42.85% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1782      +/-   ##
==========================================
+ Coverage   90.27%   94.85%   +4.58%     
==========================================
  Files          58       39      -19     
  Lines        5182     2859    -2323     
==========================================
- Hits         4678     2712    -1966     
+ Misses        504      147     -357     

Impacted Files	Coverage Δ
lib/core/util.js	81.32% <33.33%> (-16.61%)	⬇️
lib/api/api-pipeline.js	100.00% <100.00%> (ø)
lib/api/api-stream.js	100.00% <100.00%> (ø)
lib/api/readable.js	82.75% <100.00%> (-8.63%)	⬇️
lib/mock/pending-interceptors-formatter.js	100.00% <100.00%> (ø)
index.js	80.00% <0.00%> (-20.00%)	⬇️
lib/handler/RedirectHandler.js	83.75% <0.00%> (-11.25%)	⬇️
lib/mock/mock-utils.js	93.95% <0.00%> (-6.05%)	⬇️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jimmywarting]

I would prefer if you just used node:stream instead of depending on a npm package

[ronag]

I would prefer if you just used node:stream instead of depending on a npm package

Why?

[jimmywarting]

I would prefer if you just used node:stream instead of depending on a npm package

Why?

• Fewer dependencies
  • faster to download
• node:stream is always up to date with newest features
• readable-stream have lagged behind in features
• readable-stream also depends on buffer which itself haven't been updated for 2y
• so it will use a npm:buffer instead of nodes which is also slower than node:buffer when it comes to things like buf.toString() as it dosen't use native code and so on.
• every sub dependency you have no control over is a potential security risk

[ronag]

@mcollina wdyt?

[mcollina]

Why did you want to do this change in the first place?

[ronag]

Make sure we have the latest stream fixes even when not running latest version of Node. The same reason why readable-stream exists in the first place.

[ronag]

• node:stream is always up to date with newest features

This is a truth with modification. Only if/when you are running latest version of Node. Otherwise, the opposite should be true.

• readable-stream have lagged behind in features

I think and hope that situation is no longer true.

• readable-stream also depends on buffer which itself haven't been updated for 2y

Yea, I guess that is a problem.

• so it will use a npm:buffer instead of nodes which is also slower than node:buffer when it comes to things like buf.toString() as it dosen't use native code and so on.

Also true.

[jimmywarting]

also it uses abort-controller polyfill instead of using built in whenever available. and it too lacks things like AbortSignal.prototype.reason

i don't know but how compatible it is with built in node:http and other core features about accepting 3th party abort signals?

The same reason why readable-stream exists in the first place.

I mostly saw it as "just to bring it outside of nodejs" and was only mostly ment for browserify and webpack... all those legacy package never updated their own code after a while and those started to lack newer features such as async iterable

[ronag]

I mostly saw it as "just to bring it outside of nodejs". all those legacy package never updated their own code after a while and those started to lack newer features such as async iterable

For me. I want to have all the latest fixes and features from Node streams even when running LTS. I prefer using both undici and streams from npm instead of core for this reason.

[ronag]

Would be nice if it only used the polyfills when required, i.e. running an older Node version than expected.

[mcollina]

@jimmywarting those sounds good improvements to do to readable-stream. Maybe open a few issues there / or send PRs?

[ronag]

So it seems to me like this could be a good idea but would require some more things from readable-streams:

• interop broken readable-stream#501
• Only install/use events package when Node version < expected
• Only install/use process package when Node version < expected
• Only install/use buffer package when Node version < expected
• Only install/use abort-controller package when Node version < expected

Would peer dependencies help with the former 2?

[jimmywarting]

Maybe open a few issues there / or send PRs?

I'm not so keen at working on buffer and/or stream. i have a huge cross compatible code mindset, such as (async)Iterators + uint8array instead (and now web streams). also got a huge bundlephobia for large packages 😝

But you can go ahead and do that...
conditional import when required is a 👍

I just recently did a config.js in one of my project that only exported the things i needed to work with such as:

const config = {
  ReadableStream: globalThis.ReadableStream,
  WritableStream: globalThis.WritableStream,
  TransformStream: globalThis.TransformStream,
  Promise: globalThis.Promise,
  DOMException: globalThis.DOMException,
  Blob: globalThis.Blob,
  File: globalThis.File
}

export default config

and if developers wanted to they could override all those things before i even start using anything of it...

import config from 'mod/config.js'
config[xyz] = zxy
import mod

so if they needed to support older version then they could bring their own polyfill and the version they needed/wanted/or already had