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.

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

fix: don't require stream package in codebase #520

Merged
merged 4 commits into from Jun 29, 2023

Conversation

jeswr
Copy link
Contributor

@jeswr jeswr commented May 15, 2023

Closes #516.

Supercedes #517 and #519.

This replaces require('stream') with require('../../lib/stream.js') in the build script so that the stream module is never imported.

Since #517 I've moved the regression test to a new folder in the root since the other test fixtures are being copied from the nodejs codebase; wheras the one I add is specific to readable-stream.

@@ -0,0 +1,77 @@
import { exec } from 'child_process'
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 27 (note this is a copy /paste of a subset of the code in the nodejs fixtures)

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

@jeswr
Copy link
Contributor Author

jeswr commented May 22, 2023

Bump @vweevers @benjamingr

@vweevers
Copy link
Contributor

I don't have time to review.

@vweevers vweevers removed their request for review May 22, 2023 17:32
@jeswr
Copy link
Contributor Author

jeswr commented May 29, 2023

@mcollina it doesn't look like the others have time to review so can this be released?

@rubensworks
Copy link

Would it be possible to merge this package? Many of our downstream users are running into issues because of this

Happy to help where possible!

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

@mcollina mcollina merged commit 8c9c011 into nodejs:main Jun 29, 2023
115 checks passed
@jeswr jeswr deleted the fix/readable-stream-no-stream-import branch June 29, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants