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 incompatible optional dependencies #3738

Closed
gaearon opened this issue Jun 27, 2017 · 58 comments
Closed

Don't warn about incompatible optional dependencies #3738

gaearon opened this issue Jun 27, 2017 · 58 comments

Comments

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 27, 2017

Currently Yarn behavior matches npm in that it warns about optional dependencies that are incompatible (and thus skipped). fsevents on Windows is a good example of this.

Since the warning is almost never actionable, and looks a bit frightening to beginners, I propose that we downgrade its log level. I'm not sure if Yarn even has a concept of log levels, but I wouldn't want to see this warning unless I'm specifically debugging Yarn itself.

Corresponding npm issue has wide support (although it got autoclosed): npm/npm#11632

@ghost
Copy link

@ghost ghost commented Jun 28, 2017

Seems like an appropriate use of warn to me.

@wtgtybhertgeghgtwtg
Copy link
Contributor

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Jun 28, 2017

Wouldn't it be better to fix the thing that is giving the warning instead of just hiding it?

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented Jun 28, 2017

@wtgtybhertgeghgtwtg

What do you mean by "fix"? As explained in npm/npm#11632, everything is working as intended. Some packages depend on fsevents specifically on macOS (because it offers superior experience) but fall back to other behavior on Windows/Linux. The fact that fsevents is incompatible with Windows should not be surfaced to the users: it's just a platform-specific implementation detail of a dependency. There's nothing actionable for users there (and nothing to be alarmed about).

@wtgtybhertgeghgtwtg
Copy link
Contributor

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Jun 28, 2017

I would say that requiring a dependency and adding binaries for a specific OS should not be how it works.
I'm sure that there are other packages that make this issue pop up, but fsevents is the most predominate one, getting issues in babel, webpack, your create-react-app, and I sure very soon sane and jest. I think that it would be more productive to find a better file watching solution instead of changing the behavior of a package manager because of the shortcomings of one package.

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented Jun 28, 2017

If you have an alternative suggestion about what packages using fsevents (or fsevents itself) could do, I'd like to hear that! I don't think there's anything "wrong" about having a package that's OS-specific though. It solves a real problem, and some problems are platform specific.

@bestander
Copy link
Member

@bestander bestander commented Jun 28, 2017

Looks like opinions are split.
I suppose warning on a non actionable thing could be annoying.
On the other hand similar warnings, e.g. "this module does not work on Node.js < 4", are quite useful.
Is there a settings people can turn on to disable warnings?

@ghost
Copy link

@ghost ghost commented Jun 29, 2017

@gaearon given this is an open source repo, how about a PR to add a CLI flag to suppress by log level. It would be very incorrect to paper over a downstream issue in the packages you're referencing whether it be a superior computer or just a $35 RPi which can do most of the same stuff.

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented Jun 29, 2017

@jhabdas

how about a PR to add a CLI flag to suppress by log level

I would have loved to send a PR but, like @bestander, I also work full time on maintaining other open source projects, and unfortunately don’t have the time to work on this particular feature. I raised an issue to discuss whether it is desirable or not.

It would be very incorrect to paper over a downstream issue in the packages you're referencing whether it be a superior computer or just a $35 RPi which can do most of the same stuff.

I don’t understand this comment. I am not suggesting to “paper over a downstream issue”. fsevents is not doing anything wrong. It wants to express that it is only useful on macOS, but doesn’t exist on other systems. Other packages, like webpack, want to express that they’d like to use fsevents on macOS, but want to skip it on other systems. And this is exactly what both yarn and npm are doing.

Everything already works as intended, except that both npm and yarn also print a message that looks scary but doesn’t actually indicate a problem. But there is no reason for the user to be worried because everything works as intended. So my point is it would be nice to stop scaring users when nothing is wrong.

Sorry if I’m not being very clear. I can see now this is more controversial than I expected. Let’s not do this then. I’m not going to die on this hill. 😛

@gaearon gaearon closed this Jun 29, 2017
@glen-84
Copy link

@glen-84 glen-84 commented Jun 29, 2017

On the other hand similar warnings, e.g. "this module does not work on Node.js < 4", are quite useful.

... but that's actionable – you can upgrade your version of Node.js.

@bestander
Copy link
Member

@bestander bestander commented Jun 29, 2017

... but that's actionable – you can upgrade your version of Node.js.

OS limitation could be actionable as well.
I suppose the question is whether it is worth displaying the same warning every time you install a IO tool not on a Mac, I think we should give windows users a chance to not show them when the dependencies are optional.

I'll reopen, it seems worth discussing more.

@bestander bestander reopened this Jun 29, 2017
@gaearon
Copy link
Contributor Author

@gaearon gaearon commented Jun 30, 2017

We could probably downgrade this to an "info" log level (and introduce concept of log levels like npm has).

@ghost
Copy link

@ghost ghost commented Jun 30, 2017

@Gearon Could you provide two complimentary test cases please? I read the details and heard the suggestion. But I'd like to personally take in what you consider a frightening experience for beginners for something that is "almost never actionable". You mentioned Webpack above. To me Webpack itself is frightening and I don't use it as a result. There are a lot of great things that can be done with yarn if it keeps its eyes on the right feature set during development. Is this really a suggestion you feel helps achieve those goals, or are you climbing someone else's hill?

@dmbdesignpdx
Copy link

@dmbdesignpdx dmbdesignpdx commented Jul 7, 2017

I'm having somewhat of a similar situation as described by @gaearon: I'm getting warnings on dependencies that themselves use older dependencies and I guess they just haven't upgraded?

When I run yarn upgrade I get this at the start:

warning gulp > vinyl-fs > glob-stream > minimatch@2.0.10 ...
warning gulp > vinyl-fs > glob-watcher > gaze > globule > minimatch@0.2.14 ...
warning gulp > vinyl-fs > glob-watcher > gaze > globule > glob > graceful-fs@1.2.3 ...

But when I look at the list that follows, those dependencies are up-to-date.

I agree with @gaearon, seeing a warning is a bit scary especially to me since I'm relatively new to using Node and have migrated over from using npm to Yarn. But it's also a bit misleading to see a warning message as it implies that it's something that I can fix.

In my opinion, I think that these warnings should either become friendly infos or should just not displayed at all.

@wtgtybhertgeghgtwtg
Copy link
Contributor

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Jul 7, 2017

Using unsafe dependencies should give a warning, though.

@ghost
Copy link

@ghost ghost commented Jul 7, 2017

Why are we talking about scary. This is software development, not kindergarten.

@dmbdesignpdx
Copy link

@dmbdesignpdx dmbdesignpdx commented Jul 7, 2017

@wtgtybhertgeghgtwtg What throws me is that the warning asks me to update something that I can't update. Even though, in this case, I'm not the one using the unsafe dependency (I think Glob is), I do see your point and agree it should be a warning; but I think it can be worded to be more informative. Something as simple as "[dependency_A] is using an outdated [dependency_B]"—I'll know right off the bat that a) it's not my fault, the error is not on my end, and b) where to go to see if the issue has been addressed. That kind of info can save users from going down the rabbit hole of trying to figure out what's wrong on their end.

@bestander
Copy link
Member

@bestander bestander commented Jul 7, 2017

@ghost
Copy link

@ghost ghost commented Jul 7, 2017

What throws me is that the warning asks me to update something that I can't update.

If you're using a dependency you most certainly own it and everything it pulls in, and have the ability to fork and improve whatever comes with it. If it bothers you perhaps it's not worth using.

@wtgtybhertgeghgtwtg
Copy link
Contributor

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg commented Jul 7, 2017

While I would argue that

warning gulp > vinyl-fs > glob-stream > minimatch@2.0.10 ...
warning gulp > vinyl-fs > glob-watcher > gaze > globule > minimatch@0.2.14 ...
warning gulp > vinyl-fs > glob-watcher > gaze > globule > glob > graceful-fs@1.2.3 ...

tells you what you need to know, I agree that it could be worded in a way that summarizes it a bit better.

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented Jul 8, 2017

Can we keep this issue focused on one particular case please? I don't want this to turn into a bikeshed about all possible warnings.

I reported a specific warning where "fixing" the issue is impossible in principle, not even in the transitive dependencies. Everything works as intended.

Other cases described in this issue show legitimate problems that need to be reported and fixed by transitive dependencies. I don't think this is similar to the issue I described. If you feel strongly about them please file a new issue.

@ghost
Copy link

@ghost ghost commented Jul 8, 2017

I don't want this to turn into a bikeshed about all possible warnings.

If it's related the conversation should probably go right here. Please notice the number of open issues, and that this is an enhancement request and not a bug.

@voxsim
Copy link
Contributor

@voxsim voxsim commented Jul 12, 2017

@jhabdas I already answered in the PR. This is my philosophy https://stackoverflow.com/questions/153234/how-deep-are-your-unit-tests/153565#153565, but if you think that package-compatibility or that part in particular needs some tests, please open a pull request and thanks in advance :)

@ghost
Copy link

@ghost ghost commented Jul 12, 2017

I trust your good judgement but I trust SO about as far as I can throw a twig. But that's okay. I wanted to send my message for the benefit of others to see how a one line code change could have avoided this entire thread. Sometimes actions speak louder than words. Cheers.

@BYK
Copy link
Member

@BYK BYK commented Jul 12, 2017

@jhabdas as much as I appreciate your contributions, I'd like to remind you our code of conduct, specifically the following items:

  • Using welcoming and inclusive language
  • Being respectful of differing viewpoints and experiences
  • Showing empathy towards other community members
@ghost
Copy link

@ghost ghost commented Jul 12, 2017

My sincerest apologies. I will review the code of conduct and try and do better to be a good community citizen. Thanks for pointing me in the right direction, @BYK.

@jods4
Copy link

@jods4 jods4 commented Jul 14, 2017

Thanks for fixing this. 😍 🙇

It was the top voted issue in NPM by a wide margin and caused real problems, yet after 1.5 years of not addressing it their bot just auto-closed it for being too old!

That kind of project management is one of the reasons I switched to Yarn.

Seeing how easy the fix was, it's agonizing how much time and effort was wasted over this. 😢

Thanks!

@kubino
Copy link

@kubino kubino commented Jul 17, 2017

Hi, thanks for all the efforts about this. It's far more better that it will be now on my stdout instead of stderror ;). Yet still even info level will bring here tons of people. And the only output they will get after one one hour of reading related issues is that they need to live with it for the moment(who doesn't use babel) and that making platform specific optimization via optional dependencies is possible but with a cost of package managers warnings to the user. I am on a side of having this outputted only at debug level.

@BYK
Copy link
Member

@BYK BYK commented Jul 18, 2017

@kubino that's some great feedback, thank you! We are aware that moving it to info wouldn't fix it once and for all. For that @jhabdas actually spent some time to make it even better and generic and submitted an RFC. Would you be interested in providing feedback there and let us know if that RFC addresses your concerns?

@kubino
Copy link

@kubino kubino commented Jul 18, 2017

@BYK thank you for still being interested in this. I am far from being expert and maybe I got it wrong. You pointing me to RFC where the title is "make deprecated sub-package warnings more informative". I think delivering DEPRECATED warnings to the users are correct by default (although having a way to suppress particular package warnings would be probably good).
My concern was about OPTIONAL dependencies which just serves as optimization for some concrete platform, there I see very little value of polluting otherwise so lovely clean Yarn output. But as I said, I am no expert and I am not sure whether this is always so clear to determine

@BYK
Copy link
Member

@BYK BYK commented Jul 18, 2017

@kubino no need to be an expert. You know the world is full of self-proclaimed experts, so better to claim not being an expert and may be you'll end up being one 😀

Anyways, I haven't had the time to read that RFC completely yet but I think it falls under the label = "Interoperability" section. That said may be we should add an optimization section for these cases. That said let's continue the discussion on that PR and I think you should make this suggestion there and see what @jhabdas thinks about that.

@ghost
Copy link

@ghost ghost commented Jul 19, 2017

@kubino I've updated the RFC description to make it a little easier to use.

The RFC is a proposal to try and help address a number of related use cases which might pop-up by introducing a small (seemingly unrelated) feature which, when used creatively, intends to help satisfy needs such as those you've raised while also adding additional benefit to the great work Yarn has already accomplished for the community.

Be happy to discuss and use your insights to further enhance the RFC, as I've learned being more experienced can be as much or more of a liability as it can be an asset when it comes to coding.

@kubino
Copy link

@kubino kubino commented Jul 20, 2017

@BYK I know where you point ;) Sure I need to agree with both of you @jhabdas, generic solution is the only way out of this discussions. Categorization of messages is the first step which will enable everything else... let's continue the discussion in the related RFC. Thanks!

@robertjchristian
Copy link

@robertjchristian robertjchristian commented Jul 25, 2017

If it's optional "given a target OS", then it shouldn't even be WARN level. The author is saying "If MacOS then use FSEvents, otherwise don't." So if you are installing on an OS other than Mac, why warn at all? It's more like info/debug.

@mgol
Copy link

@mgol mgol commented Jul 26, 2017

@robertjchristian I think there are two aspects to this:

  1. fsevents itself doesn't work outside of macOS so an attempt to install it on such an OS should print a warning or error. Some packages using fsevents may depend on its API universally (incorrectly) without knowing they're macOS-only and breaking on other OS-es in the process.
  2. If fsevents is for some packages needed only on macOS, those packages should be able to mark fsevents as not-to-be-installed outside of macOS. A failure to install fsevents on macOS would then result in a failure but on other OS-es there wouldn't even be an attempt to install.

The core issue is that treating fsevents as an optional dependency is an incorrect approach. What's missing is the ability to mark fsevents as required on macOS and as completely excluded, not just optional, on other OS-es.

@Vanuan
Copy link

@Vanuan Vanuan commented Jul 27, 2017

Yeah, that was explained several times in the original thread, it seems that nobody listened:

npm/npm#11632 (comment)

The solution would be to add support of platform-specific dependencies.

npm/npm#11632 (comment)

the 'correct' solution is to have some method of conditional dependencies - a way for a package to indicate that one or more of it's dependencies should not be installed on some platforms. This is not the same as a package describing itself as platform specific - a package doesn't know whether it's optional or not, so all it can say is 'if you install me on the wrong platform, that's an error'.

npm/npm#11632 (comment)

Here's what npm could do:

  1. Change optional dependencies and not supported messages from WARN to INFO (what most people want).
  2. Implement platform specific dependencies (proper solution)

So instead of hiding the warning they should've properly fixed it by implementing conditional dependencies. But that requires changing package.json specification. I imagine something like this:

  "conditionalDependencies": {
    {
      "condition": { "platform": "darwin" }, // 'darwin', 'freebsd', 'linux', 'sunos' or 'win32'
      "packages": { "fsevents": "^1.0.0" }
    }
  },

Another alternative is for fsevents authors:

Make it not fail on non OSX platforms without any warnings, just INFO message about platform not supported

But that's not much different from hiding the warning on npm level.

@IAMtheIAM
Copy link

@IAMtheIAM IAMtheIAM commented Jul 21, 2018

Still no answer? That warning is so annoying.

@derekbtw
Copy link

@derekbtw derekbtw commented Jan 27, 2019

At this point I'm tempted to just buy a Mac for the sole purpose of not seeing this for another four years even though I loathe the operating system 👎

@ghost
Copy link

@ghost ghost commented Feb 2, 2019

At this point I'm tempted to just buy a Mac for the sole purpose of not seeing this for another four years even though I loathe the operating system 👎

If you're developing software professionally I couldn't recommend a better investment.

@BarishSarac
Copy link

@BarishSarac BarishSarac commented Feb 14, 2019

Why show a warning if you don't require user to do something to correct it?

Everyone Arguing to keep the warning, please take a second an think. If user cannot do anything to correct this warning, why we are showing it?

It's like your partner telling you they're hungry, you're like cook for yourself, they won't do it, you cook they won't eat, but keep bugging you, I am hungry. Every time you say hi (npm install / yarn), they say I am hungry (WARN WARN WARN).. Seems pointless to me.

@Vanuan
Copy link

@Vanuan Vanuan commented Feb 14, 2019

At this point I'm tempted to just buy a Mac for the sole purpose of not seeing this

Do you mean you won't see this on Mac?
Well, you won't unless you use docker:

MacbookPro $ docker run -ti --rm node yarn add chokidar@2.1.1
yarn add v1.13.0
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
info fsevents@1.2.7: The platform "linux" is incompatible with this module.
info "fsevents@1.2.7" is an optional dependency and failed compatibility check. Excluding it from installation.
@Vanuan
Copy link

@Vanuan Vanuan commented Feb 14, 2019

I think you'd have better luck forking chokidar and removing fsevents from there.
Recent node has great improvements wrt native fs.watch support in OS X.

Of course, you'd also have to fork dozens of packages depending on chokidar

@Vanuan
Copy link

@Vanuan Vanuan commented Feb 14, 2019

Try complaining here for a change:
https://github.com/paulmillr/chokidar/issues

@ghost
Copy link

@ghost ghost commented Feb 15, 2019

I think you'd have better luck forking chokidar and removing fsevents from there.
Recent node has great improvements wrt native fs.watch support in OS X.

Sounds like we're on the same page after all. xD

@Vanuan
Copy link

@Vanuan Vanuan commented Feb 15, 2019

Yeah, I suppose the only choice to wait until chokidar drops support for node 10 at which point it would become useless and other projects start dropping it.

@paulmillr
Copy link

@paulmillr paulmillr commented Feb 17, 2019

@Vanuan recent node.js support for fs.watch is still shit. Try actually using it across platforms.

If you want to complain about installation warnings, maybe complain to maintainers who "deprecate" packages. That's really fucking annoying. This didn't happen in the past. Fsevents is much less a problem compared to this.

@tobiasfeil
Copy link

@tobiasfeil tobiasfeil commented Jul 16, 2020

Is this just never gonna be fixed?

@aNickzz
Copy link

@aNickzz aNickzz commented Apr 28, 2021

Ok, so now how do I actually hide these "info" messages?

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

Successfully merging a pull request may close this issue.

None yet