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

chore(deps): add webpack-cli to peers #2188

Closed
wants to merge 1 commit into from
Closed

Conversation

eps1lon
Copy link

@eps1lon eps1lon commented Aug 10, 2019

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Can publish a package with this PR and see if it actually works but these kind of fixes have been done multiple times to make yarn v2 happy. Considering webpack-dev-server throws at runtime anyway if webpack-cli is missing this shouldn't bother anyone

Motivation / Use-Case

yarn v2 throws currently with

Error: A package is trying to access another package without the second one being listed as a dependency of the first one

Required package: webpack-cli (via "webpack-cli/bin/config-yargs")
Required by: webpack-dev-server@virtual:836be30d0802b4899b9a78ca9d744f43f038cccf96d6b8307cbd938d17151f1ac108d2e64290515733c51d0949e0c6d87f9a7c9e245dfe628e5c4ef98f22d752#npm:3.8.0 

Breaking Changes

I guess this new warning might be annoying if you don't run the webpack-dev-server cli. If this is a blocker I can try and see if https://github.com/yarnpkg/rfcs/blob/master/accepted/0000-optional-peer-dependencies.md works for yarn v2.

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Aug 10, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 10, 2019

Codecov Report

Merging #2188 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2188   +/-   ##
=======================================
  Coverage   96.08%   96.08%           
=======================================
  Files          34       34           
  Lines        1227     1227           
  Branches      349      349           
=======================================
  Hits         1179     1179           
  Misses         47       47           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84cb481...3ff08d0. Read the comment docs.

@eps1lon eps1lon changed the title chore: Add webpack-cli to peer dependencies chore(deps): add webpack-cli to peers Aug 10, 2019
@alexander-akait
Copy link
Member

Thanks for PR, we can't do this, it is break CFA, vue-cli, angular-cli and etc, you should put webpack-cli in your deps, also next major release was without bin, and you need webpack-cli to run webpack-dev-server using CLI

@eps1lon
Copy link
Author

eps1lon commented Aug 10, 2019

Thanks for PR, we can't do this, it is break CFA, vue-cli, angular-cli and etc, you should put webpack-cli in your deps, also next major release was without bin, and you need webpack-cli to run webpack-dev-server using CLI

@evilebottnawi Please read the PR description. I already have webpack-cli as a dependency but yarn v2 will throw errors if a package does not declare its dependencies.

Maybe @arcanis could elaborate why this is important.

@arcanis
Copy link
Contributor

arcanis commented Aug 10, 2019

I'll copy what I said on this issue:

Fwiw I don't think using an unsafe pattern known to break under some circumstances (omitting peer dependencies from the manifest) just to remove warnings is a good tradeoff, especially when those warning can already disappear for half the users by using the right feature (npm is also going to implement optional peer dependencies, btw) ...

@alexander-akait
Copy link
Member

Feel free to send a PR with peerDependenciesMeta (looks it is good idea)

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

Successfully merging this pull request may close these issues.

4 participants