Skip to content

fix: peer dependencies for webpack serve#1317

Merged
alexander-akait merged 5 commits into
nextfrom
fix-peer-dependencies
Mar 23, 2020
Merged

fix: peer dependencies for webpack serve#1317
alexander-akait merged 5 commits into
nextfrom
fix-peer-dependencies

Conversation

@alexander-akait

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

Copy link
Copy Markdown
Member

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

@rishabh3112 rishabh3112 left a comment

Copy link
Copy Markdown
Contributor

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.

@rishabh3112 rishabh3112 left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member Author

Yep, in near future

@ryanclark
ryanclark force-pushed the fix-peer-dependencies branch from a8e4f58 to eeec9ea Compare March 20, 2020 13:39
@webpack-bot

Copy link
Copy Markdown

@ryanclark Thanks for your update.

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

@rishabh3112 Please review the new changes.

Comment thread 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"

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Member Author

/cc @rishabh3112

@webpack-bot

Copy link
Copy Markdown

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

@rishabh3112 rishabh3112 left a comment

Copy link
Copy Markdown
Contributor

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

@anshumanv anshumanv left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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