search

nodejs / readable-stream

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

fix: removes `Readable.fromWeb` and `Readable.toWeb` methods #506

Closed RishabhKodes closed 1 year ago

RishabhKodes commented 1 year ago

fixes: https://github.com/nodejs/readable-stream/issues/482

The Readable.fromWeb and Readable.toWeb methods were no longer being used from here (instead being imported from require(stream') method).

This pr removes both methods during the build when the module is extracted from Node.js.

mcollina commented 1 year ago

Can you move those to the replacements.mjs file?

RishabhKodes commented 1 year ago

Can you move those to the replacements.mjs file?

Do you mean moving the code changes I made, or the doing something like this https://github.com/nodejs/readable-stream/blob/main/build/build.mjs#L94 ?

mcollina commented 1 year ago

Using the regular expressions instead.

RishabhKodes commented 1 year ago

@mcollina I've moved the changes to replacements.mjs and put the code inside a function and then imported it in the build.mjs file, PTAL let me know if that's fine.

RishabhKodes commented 1 year ago

@mcollina is the windows-latest, 18.x test flaky? I've seen it fail in a couple of other pull requests as well.

RishabhKodes commented 1 year ago

@benjamingr @mcollina the Node.js / Node.js (windows-latest, 18.x) test is still failing after the re-run.

RishabhKodes commented 1 year ago

@mcollina @benjamingr, an update on this pr? Has been idle for a long time.

mcollina commented 1 year ago

Sorry about it, it was buried down my github notifications.