-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Bump and control the version of lodash in dev-dependencies #4988
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
Conversation
Wondering if we should wait until lodash/lodash#4844 is resolved? |
@@ -152,6 +152,7 @@ | |||
"karma-spec-reporter": "0.0.32", | |||
"karma-verbose-reporter": "0.0.6", | |||
"karma-viewport": "^1.0.6", | |||
"lodash": "^4.17.19", |
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.
do we need to add it here? Isn't it sufficient to have lodash bumped in package-lock.json
?
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 think it is better to keep it this way e.g. in case one wants to regenerate the lock as well the for the yarn upgrade.
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 just don't want us to be in the habit of including sub-dependencies in package.json
, is this one special in some way? Are we waiting for something that will let us remove this?
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.
Until all those sub-dependencies switch to force using newer version, I think we need to control the version at root.
We have few other dev-dependencies e.g. acorn
, static-eval
that controlled like this.
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.
OK, fair enough.
Seems like if our builds and tests run, that issue isn't a problem for us, right? And anyway it's not clear yet that they're in any rush to fix it. |
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.
💃
Addressing 12 audit warnings https://www.npmjs.com/advisories/1523.
@alexcjohnson