-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
feat: serve integration #1712
feat: serve integration #1712
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.
Let's do some improvements to avoid npm
/yarn
/pnp
problems with https://github.com/webpack/webpack-cli/blob/next/packages/serve/package.json#L17:
- Remove
webpack-dev-server
frompeerDependencies
"peerDependenciesMeta": { "webpack-dev-server": { "optional": true } }
- Add
let webpackDevServer;
try {
webpackDevServer = require.resolve('webpack-dev-server')
} catch (error) {
throw new Error(`You need to install `webpack-dev-server` for running `webpack server`.\n${error}`);
}
Maybe we should improve error message, feel free to do it
@rishabh3112 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @evilebottnawi Please review the new changes. |
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.
One fix
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.
After open issues, feel free to merge when CI will be green
55867d5
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.
/cc @webpack/cli-team need review
What kind of change does this PR introduce?
Serve integration
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
No
Summary
Integrated serve regardless some issues left to be addressed.
Does this PR introduce a breaking change?
No
Other information
@evilebottnawi 's comment #717 (comment)