-
-
Notifications
You must be signed in to change notification settings - Fork 614
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
chore(upgrade): webpack 4.2 and other dependencies #362
Conversation
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
"prettier-eslint-cli": "^4.7.1", | ||
"schema-utils": "^0.4.5", | ||
"webpack": "^4.1.1", | ||
"webpack": "^4.2.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.
This is something that bugs me. Shouldn't we have ^4.0.0
and that's it? If not, why?
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 does it bugs you?
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.
The cli
should have a loose dependency from webpack
as far as I understood. The cli
uses only some utilities from webpack
. So I still have some unclear thoughts about how this relation is inside the cli
. Just personal doubts :)
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.
About loose dependency, I think it depends on the peer dependency which is currently "^4.0.0"
.
Lines 109 to 111 in 2222f1d
"peerDependencies": { | |
"webpack": "^4.0.0" | |
}, |
IMO we should keep our project's dependencies up to date to update the lock file since all npm installation would pick from the versions specified in lock file and not the latest version based on ^
(unless you remove the lock file explicitly before npm installations).
The loose dependency should still carry as it's a peer.
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.
Thanks!
@dhruvdutt Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ev1stensberg Please review the new changes. |
Upgraded |
No description provided.