Skip to content
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

Merged
merged 5 commits into from
Mar 23, 2020
Merged

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Mar 11, 2020

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

Copy link
Member

@rishabh3112 rishabh3112 left a 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.

Copy link
Member

@rishabh3112 rishabh3112 left a 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.

@alexander-akait
Copy link
Member Author

Yep, in near future

@webpack-bot
Copy link

@ryanclark Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@rishabh3112 Please review the new changes.

"inquirer": "6.5.1",
"webpack-cli": "^4.0.0-beta.8",
"webpack-dev-server": "^3.9.0"
"cross-spawn": "6.0.5"
Copy link
Member Author

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?

Copy link
Member

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

@alexander-akait
Copy link
Member Author

/cc @rishabh3112

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

Copy link
Member

@rishabh3112 rishabh3112 left a 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

Copy link
Member

@anshumanv anshumanv left a 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.

@alexander-akait
Copy link
Member Author

@anshumanv it is peerDependencies, developer should install webpack-dev-server manually, because we will support webpack-dev-server@3 and webpack-dev-server@4 (next)

@anshumanv
Copy link
Member

@anshumanv it is peerDependencies, developer should install webpack-dev-server manually, because we will support webpack-dev-server@3 and webpack-dev-server@4 (next)

Okay makes sense 👍

@alexander-akait alexander-akait merged commit f8ec203 into next Mar 23, 2020
@alexander-akait alexander-akait deleted the fix-peer-dependencies branch March 23, 2020 10:24
@alexander-akait
Copy link
Member Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants