search

nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

Context behind export of `isDisturbed` from top-level? #511

Closed nadhifikbarw closed 1 year ago

nadhifikbarw commented 1 year ago

Hi, I'm curious about potentially deeper technical context or historical reasoning behind why isDisturbed is exported from top level on condition where Stream built-in lib is present. When as of 16-x docs it has always been exposed as static inside Readable class?

https://github.com/nodejs/readable-stream/blob/232e71154dbdc34f6a97a0449f902455ec495373/src/index.js#L3-L13

My use case right now only to update d.ts file for readable-stream 4.x.x that will mostly rely on just re-exporting existing type from stream.d.ts and I haven't checked built-in implementation that might exposed isDistrubed on top-level but undocumented.

Since v16.8.0 is the first Node version where that feature is released according to official docs, wouldn't that mean Stream.isDisturbed is still missing from Node 12-15 versions which are the support range for 4.x ?

Unless I'm oblivious on some part of the build process that might goes on that influence the end code produced for this library or I completely misunderstood module resolution on line 3 of require('stream') where it doesn't actually load from built-in module implementation. Would love to know the details so I can accurately reflect the code on the Typescript ambient module definition!

mcollina commented 1 year ago

It would be best if you ignored everything inside the process.env.READABLE_STREAM === 'disable' block when preparing the types.

isDisturbed is correctly exposed as require('readable-stream').isDisturbed() because require('readable-stream') exposes Readable and not Stream as its entry point (mostly for historical reasons).

nadhifikbarw commented 1 year ago

Thanks! appreciate the additional context, will close the discussion.