Don't warn about incompatible optional dependencies
gaearon opened this issue · 58 comments
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
Seems like an appropriate use of warn to me.
Wouldn't it be better to fix the thing that is giving the warning instead of just hiding it?
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).
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.
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.
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?
@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.
@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. 😛
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.
... 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.
We could probably downgrade this to an "info" log level (and introduce concept of log levels like npm has).
@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?
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.
Using unsafe dependencies should give a warning, though.
Why are we talking about scary. This is software development, not kindergarten.
@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.
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.
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.
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.
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.
Sounds good. I'll unsubscribe but I hope this discussion stays focused in the future. Thanks for participating!
Maintainers, please close this. There's no one to follow through.
Could you provide two complimentary test cases please?
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.
Please notice the number of open issues, and that this is an enhancement request and not a bug.
Maintainers, please close this.
@jhabdas, I appreciate your effort to get me to contribute more of my time to this issue, and pointing out that Yarn team has enough on their plate, but there is no need to keep reiterating this. This is not a very friendly behavior.
I apologize but as I already said before, I can’t really dedicate time to this issue. I trust Yarn maintainers to make the right choices in prioritization. If they think the issue is too low priority, they will rightfully ignore or close it.
Please forgive if I misunderstand, and you’re involved in maintaining this repository. In this case please feel free to close this issue.
But I'd like to personally take in what you consider a frightening experience for beginners for something that is "almost never actionable".
Installing create-react-app and then running create-react-app myapp will produce a warning as part of the installation:
warning fsevents@1.1.2: The platform "win32" is incompatible with this module.
info "fsevents@1.1.2" is an optional dependency and failed compatibility check. Excluding it from installation.
The npm warning is even scarier:
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.0.14 (node_modules/react-scripts/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.0.14: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^1.0.0 (node_modules/react-scripts/node_modules/chokidar/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.0.14: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
Luckily npm lets us specify the log level, which Yarn doesn't. Unfortunately we have to ignore all npm warnings because of this.
Why is this warning a problem?
To an experienced user like you, it is not a problem. But Create React App is the entry point into React and npm ecosystem for many people. For some people, it’s an entry point into web development.
They don’t know what fsevents is. The message reads as though Create React App is incompatible with Windows. This is their takeaway. It’s great if everything works later, but if something breaks, they don’t always report an issue because they get the idea that it’s not supposed to work thanks to these warnings. I’m trying to combat this impression by this proposal.
Why are we talking about scary. This is software development, not kindergarten.
The assumption that people only get scared in kindergarten is perplexing to me. There are plenty things people are scared of throughout their lives. This includes both mental conditions like anxiety, and other fears (e.g. fear you’re not good enough at your job).
It might seem unrelated to JavaScript tools, but it’s really not. There are two separate problems here:
-
Unclear unactionable warnings make people anxious. They make them trust their tools less, and make them confused whether they did something wrong even if they didn’t. This is contributing to cognitive drain described so well by Kathy Sierra.
-
Unactionable warning noise makes people develop blind spot for warnings. People learn to ignore them. Every unactionable warning contributes to this cost. We’ve seen happen many times with different tools, not just Yarn. It’s “the boy who cried wolf” scenario. As a result, in the future people fail to diagnose issues and spend their and maintainers’ time because they missed an important warning in the sea of unimportant ones.
I hope this clarified why I think it’s worth addressing. I won’t, however, be participating in this discussion further because you seem to be very passionate about shooting this proposal down, and I’m not willing to invest more personal energy in discussing it with you.
This is the depth of information I was hoping to draw out @gaearon. Thank you for taking the time to explain as it seems your empathy for the learner is shining and that's commendable.
I feel it's important we teach out learners to fish for themselves. And sometimes that means not shielding them from their fears but instead encouraging them to face fear head on.
In the case of an fsevents warning, even as an optional dependency, learners should be encouraged to observe the warning and seek to understand it. Otherwise they will not grow.
A quick search of Stack Overflow, for example, presents the following solution to handling the warning message described: https://stackoverflow.com/a/42938398/712334.
If cognitive overhead becomes a concern, then I would encourage an individual to seek a different tool for creating their single-page apps or to begin their knowledge journey somewhere more suitable for gaining the fundamentals of the Web.
@glen-84 do you care to share your opinion, or just cast your thumb around?
Sure.
- Showing a warning about something that is not actionable and does not cause any issues results in:
- Wasted time by developers first having to find out why it's happening, and then having to ignore it each time (and ignoring warnings is a bad thing)
- Wasted time by Yarn/npm developers, having to explain why it happens
- Downstream tooling errors
- etc.
- If you combine the votes from the npm issue, nearly 150 users agree.
- The npm developers have already accepted the change, in theory.
I down-voted two of your comments, because IMO they add no value to this discussion.
I agree that warnings that are not actionable are not useful and they can also be unnecessarily scary/noisy/. Let's go with @dmbdesignpdx's suggestion as @bestander suggested?
PR's welcome.
Wasted time by developers first having to find out why it's happening, and then having to ignore it each time (and ignoring warnings is a bad thing)
You don't ignore it. You learn why it's happening and take action. Sometimes that will lead to another toolset or an issue in the repo addressing the root cause as suggested by @Vanuan in the related NPM issue.
Wasted time by Yarn/npm developers, having to explain why it happens
It's not Yarn's responsibility to address issues from individual repositories or package managers. And NPM is not the only packager out there. And personally I'd prefer to see Yarn grow to replace Composer and fill the void Bower just left the community rather than get distracted by red herrings.
If you combine the votes from the npm issue, nearly 150 users agree.
That's argumentum ad populum. Design by committee never works out well for anyone.
Removed the cat-discusson label since we had our discussion and now we have an actionable thing to do: make the warning more useful by stating which package depends on that other deprecated package.
@jhabdas I think we agree on the "learning" and "taking action" parts. The part we seem not to agree too much is how we do it and whether it's yarns duty or not. For now I think the more helpful yarn is to its users, the better it is. That said I wouldn't prioritize this highly to take resources away from more critical fixes and updates, hence we defer to the community for a PR.
If anyone wants to discuss more on this issue, let's move it to Discord. You can ping me there directly to get my attention. 💓
You learn why it's happening and take action
What action should we take?
It's not Yarn's responsibility to address issues from individual repositories or package managers.
It is Yarn and npm developers who will see repeatedly reported issues regarding this matter, and will have to spend time with unnecessary issue triage instead of working on new features and bug fixes.
That's argumentum ad populum. Design by committee never works out well for anyone.
It's all we have right now, and if you disagree with these changes, then your down-vote on the issue would help the maintainers with their decision making.
Okay @glen-84 kindly reminded me that the original issue was not about sub dependencies and the informative messages but simply not warning about a package that cannot be installed on a specific platform.
So now the action for this specific ticket is to simply lower it from a warning to info or debug since it doesn't add value. Objections? Moved the other part to #3869.
@gaearon A one line code change without unit test was made to resolve this issue. Please remember that.
@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 :)
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.
@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
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.
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!
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.
@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?
@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
@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.
@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.
@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!
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.
@robertjchristian I think there are two aspects to this:
fseventsitself doesn't work outside of macOS so an attempt to install it on such an OS should print a warning or error. Some packages usingfseventsmay depend on its API universally (incorrectly) without knowing they're macOS-only and breaking on other OS-es in the process.- If
fseventsis for some packages needed only on macOS, those packages should be able to markfseventsas not-to-be-installed outside of macOS. A failure to installfseventson 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.
Yeah, that was explained several times in the original thread, it seems that nobody listened:
The solution would be to add support of platform-specific dependencies.
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'.
Here's what npm could do:
- Change optional dependencies and not supported messages from WARN to INFO (what most people want).
- 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.
Still no answer? That warning is so annoying.
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 👎
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.
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.
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.
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
Try complaining here for a change:
https://github.com/paulmillr/chokidar/issues
I think you'd have better luck forking chokidar and removing
fseventsfrom there.
Recent node has great improvements wrt nativefs.watchsupport in OS X.
Sounds like we're on the same page after all. xD
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.
@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.
Is this just never gonna be fixed?
Ok, so now how do I actually hide these "info" messages?