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

Don't warn about unmet/missing package constraints on install #4064

Open
BYK opened this issue Aug 1, 2017 · 14 comments
Open

Don't warn about unmet/missing package constraints on install #4064

BYK opened this issue Aug 1, 2017 · 14 comments
Assignees

Comments

@BYK
Copy link
Member

BYK commented Aug 1, 2017

This may sound a bit controversial: we should not display warnings when there are unmet package dependencies when running yarn install.

Reasoning

The users of yarn install are usually the consumers of the package and displaying them warnings about certain package version issues is just noise, they are not actionable. These are only actionable when you are adding or upgrading packages.

Proposal

Reduce the warnings to info or debug when running yarn install and make them warnings (or errors?) when running yarn add or yarn update

@BYK
Copy link
Member Author

BYK commented Aug 1, 2017

@yarnpkg/core would love to hear opinions on this.

@BYK BYK self-assigned this Aug 1, 2017
@arcanis
Copy link
Member

arcanis commented Aug 1, 2017

These are only actionable when you are adding or upgrading packages.

I suspect a fair amount of our users never use add and remove, and instead manually update their package.json then run install.

displaying [these] warnings about certain package version issues is just noise

Which warnings exactly are you refering to? The ones you're refering to basically mean "btw, your application might possibly crash, since the versions don't match", so I'm not sure they would qualify as noise.

That being said, I agree we report some things as warnings when the things we warn about can't have any visible effect. The license warning comes to mind, but there might be more.

@BYK
Copy link
Member Author

BYK commented Aug 1, 2017

I suspect a fair amount of our users never use add and remove, and instead manually update their package.json then run install.

Ah, great point. Glad you mentioned that. That means we should handle this case too: "when running install but only if package.json is updated (doesn't match yarn.lock)?"

Which warnings exactly are you refering to? The ones you're refering to basically mean "btw, your application might possibly crash, since the versions don't match", so I'm not sure they would qualify as noise.

Even yarn project itself has this: warning "[email protected]" has incorrect peer dependency "ajv@>=4.10.0". This is not actionable even on my side and this warning should only be displayed to me if I can do something about it or if I'm changing something right at that moment.

That being said, I agree we report some things as warnings when the things we warn about can't have any visible effect. The license warning comes to mind, but there might be more.

Yeah there are more and I think we should reduce our usage of warnings and replace them with info or debug to reduce our output, especially on automated systems like CI.

@arcanis
Copy link
Member

arcanis commented Aug 1, 2017

"when running install but only if package.json is updated (doesn't match yarn.lock)"

Looks correct.

Even yarn project itself has this: warning "[email protected]" has incorrect peer dependency "ajv@>=4.10.0". This is not actionable even on my side and this warning should only be displayed to me if I can do something about it or if I'm changing something right at that moment.

Even if it's not directly actionnable I'd argue it's still pretty important. The bug might not be in our package, but it still exists. At the very least, a valid action is to forward this error to the package maintainer (in table case, this is gajus/table#49, in the webpack case it's been solved starting from 3.0.0).

My current train of thought is:

  • Any warning that can have a visible effect on how the program behave should stay a warning
  • Anything that doesn't have any visible effect (such as the license) should be an info
  • Any warning should be silenceable on a per-warning basis

I would be in favor of whitelisting the warnings that need to be silenced. For example the following content in a yarn-warnings.txt file:

// remove when https://github.com/gajus/table/issues/49 is fixed
warning "[email protected]" has incorrect peer dependency "ajv@>=4.10.0"

What do you think? Two policies would be applicable on this file (possibly toggled on-off via a yarnrc flag): strict or not. In strict mode, install would have to yield those warning, no more no less, or the install would abort. In non-strict mode, any extra warning would be printed, and any missing warning would also be reported, but the install would complete.

As an extra, maybe we should make it more obvious who the point of contact is - in afv-keywords case, it would be the github repository, for example. We should print that, so that it would make the error actionnable.

@BYK
Copy link
Member Author

BYK commented Aug 1, 2017

Even if it's not directly actionnable I'd argue it's still pretty important. The bug might not be in our package, but it still exists.

This may not be a bug and even then, if it is not actionable, there's no point in showing it (except, may be in debug mode).

At the very least, a valid action is to forward this error to the package maintainer (in table case, this is gajus/table#49, in the webpack case it's been solved starting from 3.0.0).

This is an interesting thought but it would quickly turn into spam? Also what about private packages etc.?

Any warning that can have a visible effect on how the program behave should stay a warning

I kind of agree but you cannot know if this would have a "visible" effect or not. I'd go with "anything that is not actionable and not breaking builds should remain silent (info/debug)".

Anything that doesn't have any visible effect (such as the license) should be an info

See above.

Any warning should be silenceable on a per-warning basis

This is interesting. We may wanna start assigning ID's/error-codes to warnings like JSHint did and also have mechanisms to silence these but I have a feeling that it may become too hairy and out of scope for yarn. Also it may result in hiding important warnings if this is configured incorrectly.

What do you think? Two policies would be applicable on this file (possibly toggled on-off via a yarnrc flag): strict or not. In strict mode, install would have to yield those warning, no more no less, or the install would abort. In non-strict mode, any extra warning would be printed, and any missing warning would also be reported, but the install would complete.

Interesting, since I've been thinking about something similar but separately from this. I think we need a command-line flag (that can also be added to .yarnrc to have a strict-mode which would make all warnings errors.

As an extra, maybe we should make it more obvious who the point of contact is - in afv-keywords case, it would be the github repository, for example. We should print that, so that it would make the error actionnable.

+1

@pixelass
Copy link

pixelass commented Aug 2, 2017

As far as I see it, it is currently impossible to run yarn without a bunch of unmet peer dependencies.
These warnings are very irritating. I thought I know how npm/yarn works but these error are so irritating that I am not sure if there is an actual issue or if I can simply ignore the warnings. All warnings come from sub-dependencies so I can't do anything about them except for manually installing all those dependencies (which is totally stupid because I don't need them as a top level dependency)

@arcanis
Copy link
Member

arcanis commented Aug 4, 2017

An extra thing I think that we could/should do (in the process of improving the publish process) would be to have an actual validation. It shouldn't be possible to push a package that would unexpectedly trigger such warnings.

@kaylie-alexa
Copy link
Member

+1 for reducing the noise. Maybe we can introduce a summary version like yarn check does so that they're not completely lost?

info Found 2 warnings.

@favna
Copy link

favna commented Apr 7, 2018

since I haven't seen it mentioned yet I think it's also worth mentioning that a --verbose flag would pretty much "fix" the display of errors/warnings.

no --verbose ==> Only display @kaylieEB 's suggest of info Found 2 warnings.
with --verbose ==> Display the entire warnings/errors bit

This would be different from the --silent flag as --silent displays no install log at all while omitting verbose would still display info such as

[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
[4/5] Linking dependencies...
[5/5] Building fresh packages...

@Izhaki
Copy link

Izhaki commented Jul 6, 2018

Our thoughts:

  • Yarn anyhow installs the missing peer dependencies. So they are not really 'missing'.
  • Having to add peer dependencies to package.json:
    • Inflates it.
    • If you remove the original package, you are likely to leave the peer dependency there (you'd need to check each package's peer dependencies, and then determine whether they are used elsewhere). That's a lot of cognitive cycles wasted on something that is anyhow automated by yarn.
    • Your installed version of the peer dependency is yet another dependency to manage (version wise).

As for the message:

  • As noted above, there isn't really anything to warn about - yarn installs peer dependencies.
  • Found 2 warnings - as a user:
    • What are the warnings?
    • Am I suppose to ignore them?
    • How can I tell?
    • So that message actually conceals valuable information that you should either act upon or ignore. But you need to know what the warning are.
  • Something like 5 missing peer dependencies installed could work?

I don't know if --verbose should show the missing dependencies (I reckon people using --verbose to debug only, so that seems reasonable) or a flag like --showMissing.

@HosseinAgha
Copy link

It takes us hours to fix some of the peer dependency warnings in a big monorepo. This is specially annoying when you are using yarn workspaces. We have lots of tests for our projects and all tests are passing so definitely something is wrong about these warnings or how people develop libraries!

@TekSiDoT
Copy link

I'm surprised this issue hasn't gotten more attention over the years:

  • These potentially long lists of warnings for missing peer dependencies create a lot of noise.
  • Spamming warnings promotes the ignorance of warnings and hides potentially important information.
  • @Izhaki has shown that manually handling these warnings is not an option

the --showMissing flag seems like an easy enough fix for this issue, are there any plans to consider this ?

@TekSiDoT
Copy link

TekSiDoT commented Jul 23, 2020

I've taken a closer look at this issue.

  • Most (if not all) of the development effort is now focused on Yarn2
  • Yarn2 handles these warnings a lot better (they're still there though)

Internally, we're mostly using yarn as a checked in .js 'binary' for handling offline CI installs. Because there probably won't be any major changes to yarn v1.22, we simply removed the respective warning output locally:

yarn/src/package-linker.js

Lines 661 to 666 in 98d51d8

this.reporter.warn(
this.reporter.lang(
peerError,
`${refTree.join(' > ')} > ${pkg.name}@${pkg.version}`,
`${peerDepName}@${range}`,
),

@wiredmatt
Copy link

wiredmatt commented Jun 16, 2022

I seem to still be experiencing the same, our CI/CD logs are bloated with irrelevant warnings, what we did is to just filter them out with pipes and grep:

{
    "yarn": "yarn install 2> >(grep -v warning 1>&2)"
}

But this doesn't solve the DX issue, when I run yarn add dep I'm still getting a bunch of warnings for unmeet dependencies and the likes.

Shouldn't this be handled with .yarnrc.yml? It's supposedly covered here actually, but it's not working for me, at least not in yarn workspaces (monorepo).

It somewhat works with this:

logFilters:
  - level: discard
  - pattern: "*unmet peer dependency*"

But, when adding new dependencies the messages will appear again

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

No branches or pull requests

9 participants