-
Notifications
You must be signed in to change notification settings - Fork 230
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
fix: don't require stream
package in codebase
#520
Conversation
@@ -0,0 +1,77 @@ | |||
import { exec } from 'child_process' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Bump @vweevers @benjamingr |
I don't have time to review. |
@mcollina it doesn't look like the others have time to review so can this be released? |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Closes #516.
Supercedes #517 and #519.
This replaces
require('stream')
withrequire('../../lib/stream.js')
in the build script so that thestream
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
.