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

Removed event-stream dependency #164

Merged
merged 4 commits into from
Nov 30, 2018
Merged

Removed event-stream dependency #164

merged 4 commits into from
Nov 30, 2018

Conversation

aal89
Copy link
Contributor

@aal89 aal89 commented Nov 27, 2018

What does this PR do?

Locks Removes the event-stream dependency and updates nodemon dependency to 1.18.7 to mitigate a possible attack. Also included the package-lock.json file to have reproducible builds. This most probably helps to stay safer.

Background
dominictarr/event-stream#116

Resolves
#163

@TwoAbove
Copy link

#165
This one is better imo

package.json Outdated
@@ -29,7 +29,7 @@
"homepage": "https://github.com/JacksonGariety/gulp-nodemon",
"dependencies": {
"colors": "^1.2.1",
"event-stream": "^3.3.4",
"event-stream": "3.3.4",
Copy link

Choose a reason for hiding this comment

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

maybe add the = operator to be more explicit

@joebowbeer
Copy link

joebowbeer commented Nov 27, 2018

Or update to [email protected] (a la #165 ), which is also clean of the bogus flatmap-stream dep.
Better yet, by the burn-me-once principle: move off of event-stream?

@yoelfme
Copy link

yoelfme commented Nov 27, 2018

Or update to [email protected] (a la #165 ), which is also clean of the bogus flatmap-stream dep.
Better yet, by the burn-me-once principle: move off of event-stream?

yep, that sounds better

@aal89
Copy link
Contributor Author

aal89 commented Nov 28, 2018

I have updated the PR. Ever since @joebowbeer commented: 'burn-me-once principle: move off of event-stream', I checked if it was used anywhere and ran the test, but couldn't find any problems after removing it entirely. Could people verify? Then I think this is the best fix (reference)? Don't require it if you don't need it.

@aal89 aal89 changed the title Locked event-stream to v3.3.4 Removed event-stream dependency Nov 28, 2018
@simison
Copy link

simison commented Nov 28, 2018

This looks great now, @aal89 👍

Indeed looks like event-stream isn't used directly by gulp-nodemon so it's safe to remove.

@simison
Copy link

simison commented Nov 28, 2018

@JacksonGariety hope you're not very busy — would be fantastic to get this change in and new version bumped in NPM.

Thanks for all your work! ❤️

@rxmarbles
Copy link

I was curious why event-stream is a dependency, glad this is being removed in this PR. 👍

@aal89
Copy link
Contributor Author

aal89 commented Nov 28, 2018

@rkmarks I looked through the history and it seems that it once had a purpose, however somewhere down the road it had been removed, just never as a dependency.

map-stream was being replaced by event-stream here and actually using a tilde for version selection. It later got updated and changed to a carot for version selection here. Continuing down the road it got updated, eventually requiring the dirty version. (disc: not implying the tilde/carot thing is bad).

@simison simison mentioned this pull request Nov 28, 2018
@yoelfme
Copy link

yoelfme commented Nov 28, 2018

that's all @aal89, thanks for your help. @JacksonGariety woulb be great if we can get these changes in a new version

@ColemanGariety ColemanGariety merged commit ef52e08 into ColemanGariety:master Nov 30, 2018
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.

7 participants