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

Replace event-stream by through2 #57

Merged
merged 3 commits into from Jan 21, 2019
Merged

Conversation

louis-bompart
Copy link
Contributor

event-stream, one of the dependencies of the package, have been backdoored (see dominictarr/event-stream#115).
To avoid potential issues, I would recommend using a smaller package that does just what's needed, like
through or, as proposed in this PR through2. Both are imho valid choices, I just chose through2 because it has a higher download count on npm and its latest update is more recent.

@louis-bompart louis-bompart changed the title Replace event-stream by through1 Replace event-stream by through2 Nov 26, 2018
@tatsuyafw
Copy link
Owner

@louis-bompart

Thanks for your PR!
But It seems that your PR causes errors as follows 😢

events.js:183
      throw er; // Unhandled 'error' event
      ^

TypeError: Invalid non-string/buffer chunk
    at validChunk (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/readable-stream/lib/_stream_writable.js:304:10)
    at DestroyableTransform.Writable.write (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/readable-stream/lib/_stream_writable.js:332:62)
    at write (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/vinyl-fs/node_modules/readable-stream/lib/_stream_readable.js:623:24)
    at flow (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/vinyl-fs/node_modules/readable-stream/lib/_stream_readable.js:632:7)
    at DestroyableTransform.pipeOnReadable (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/vinyl-fs/node_modules/readable-stream/lib/_stream_readable.js:664:5)
    at emitNone (events.js:106:13)
    at DestroyableTransform.emit (events.js:208:7)
    at emitReadable_ (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/vinyl-fs/node_modules/readable-stream/lib/_stream_readable.js:448:10)
    at emitReadable (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/vinyl-fs/node_modules/readable-stream/lib/_stream_readable.js:444:5)
    at readableAddChunk (/Users/hoshino/src/github.com/louis-bompart/gulp-nightwatch/node_modules/vinyl-fs/node_modules/readable-stream/lib/_stream_readable.js:187:9)

@louis-bompart
Copy link
Contributor Author

louis-bompart commented Jan 7, 2019

Hey sorry for the delay, I misread some stuff in through2, so yeah, through should work just fine.
I updated my PR accordingly, lemme know if the issue still persists.

@tatsuyafw
Copy link
Owner

Hey sorry for the delay, I misread some stuff in through2, so yeah, through should work just fine.
I updated my PR accordingly, lemme know if the issue still persists.

@louis-bompart

Thanks! Works fine!

package.json Outdated
@@ -26,7 +26,8 @@
"minimist": "^1.1.1",
"nightwatch": "^1.0.6",
"plugin-error": "^1.0.1",
"shell-quote": "^1.4.3"
"shell-quote": "^1.4.3",
"through2": "^3.0.0"
Copy link
Owner

Choose a reason for hiding this comment

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

through2 -> through ?

In addition, I think that event-stream package is no longer 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.

True! I'll did that a bit in a hurry, sorry for that.
I'll update the package.json

@tatsuyafw tatsuyafw self-requested a review January 21, 2019 13:30
Copy link
Owner

@tatsuyafw tatsuyafw left a comment

Choose a reason for hiding this comment

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

LGTM!

@tatsuyafw tatsuyafw merged commit 8155724 into tatsuyafw:master Jan 21, 2019
@tatsuyafw
Copy link
Owner

@louis-bompart Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants