-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Update lodash #1533
Update lodash #1533
Conversation
package.json
Outdated
@@ -19,7 +19,7 @@ | |||
"utility" | |||
], | |||
"dependencies": { | |||
"lodash": "^4.14.0", | |||
"lodash": "^4.17.5", | |||
"lodash-es": "^4.14.0" |
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.
Does lodash-es
need to be updated too?
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.
lodash-es
doesn't depend on lodash
, it's fine as is
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.
As far as I know, lodash-es
is built from lodash
.
Either way, it's probably a good idea to keep them synced. Could you update it to v4.17.5
? The PR will be good to go afterwards.
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.
You are right! Done!
@@ -48,7 +48,7 @@ | |||
"karma-mocha-reporter": "^2.2.0", | |||
"mocha": "^3.1.2", | |||
"native-promise-only": "^0.8.0-a", | |||
"nyc": "^7.0.0", | |||
"nyc": "^11.8.0", |
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.
I'm assuming this is because the old version of nyc
was using an older version of lodash
?
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.
that's correct
Thanks for the PR! I just added a couple of quick comments. I'm okay with merging this for a quick patch release until #1528 gets merged and released in |
Thank you @hargasinski ! I only updated packages using lodash, there are more cases that you might want to look into, but all of those are dev deps. I wasn't familiar with those tools, so I didn't push further changes and brake anything. You can try an |
Going to merge this for a patch, will remove Lodash in 3.0 |
Fixes #1532 . Updates lodash in all deps, in light of https://nodesecurity.io/advisories/577, just in case.
Pls review and merge this or #1528 , we all need it.