-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
fix: peer dependencies for webpack serve
#1317
Conversation
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.
Looks good to me.
Tip: We may reduce size of serve package by moving to enquirer
which has same API.
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.
Please rebase, next branch is passing now.
Yep, in near future |
a8e4f58
to
eeec9ea
Compare
@ryanclark Thanks for your update. I labeled the Pull Request so reviewers will review it again. @rishabh3112 Please review the new changes. |
packages/serve/package.json
Outdated
"inquirer": "6.5.1", | ||
"webpack-cli": "^4.0.0-beta.8", | ||
"webpack-dev-server": "^3.9.0" | ||
"cross-spawn": "6.0.5" |
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.
Hm, where we use cross-spawn
in out code? Maybe it should be dev dependencies?
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 only place I see it used in the repo is part of the package-utils
library
/cc @rishabh3112 |
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
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.
Looks good to me.
P.S.
A suggestion/doubt,
Isn't use of webpack serve
a little cumbersome as it has webpack-dev-server
as peer dependency. Currently using it would need following command
yarn add webpack webpack-cli webpack-dev-server @webpack-cli/serve
Instead having webpack-dev-server
as direct dependency will result in better installation behavior and DX
yarn add webpack webpack-cli @webpack-cli/serve
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.
+1 with having webpack-dev-server
in dependency in serve package, so users won't have to install it separately.
@anshumanv it is |
Okay makes sense 👍 |
Thanks |
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
No need
If relevant, did you update the documentation?
No need
Summary
Allow developers to install any webpack-dev-server version (3 or next) + move webpack-cli to peer dependencies, because it is peer dependency
Does this PR introduce a breaking change?
Yes, you should add webpack-cli and webpack-dev-server to package.json and install them if you want to use
webpack serve
Other information
No