Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from May 8, 2023
Merged

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

merged 3 commits into from May 8, 2023

Conversation

RishabhKodes
Copy link
Contributor

fixes: #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.

@RishabhKodes
removing obsolete methods from Readable class
de43dd6 
Signed-off-by: Rishabh Bhandari <rishabhbhandari6@gmail.com>
@mcollina
Copy link
Member

Can you move those to the replacements.mjs file?

@RishabhKodes
Copy link
Contributor Author

RishabhKodes commented Feb 26, 2023 •
edited

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
Copy link
Member

Using the regular expressions instead.

@RishabhKodes
moved the logic to replacements.mjs
651395a 
Signed-off-by: Rishabh Bhandari <rishabhbhandari6@gmail.com>
@RishabhKodes
Copy link
Contributor Author

@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.

mcollina
mcollina requested changes Feb 27, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was trying to say is to use the system that's already in place and not add new code, just replacements via regexps.

@RishabhKodes
using the replacements.mjs logic
13f8c64 
Signed-off-by: Rishabh Bhandari <rishabhbhandari6@gmail.com>
mcollina
mcollina approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@RishabhKodes
Copy link
Contributor Author

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

@RishabhKodes
Copy link
Contributor Author

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

@RishabhKodes
Copy link
Contributor Author

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

@mcollina mcollina mentioned this pull request Mar 20, 2023
Closed
@mcollina mcollina merged commit 29fae5b into nodejs:main May 8, 2023
11 of 12 checks passed
@mcollina
Copy link
Member

mcollina commented May 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Readable.fromWeb and Readable.toWeb do not seems to be properly implemented
3 participants
@RishabhKodes @mcollina @benjamingr