-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Node warnings bubble up and in bin scripts can't be disabled #32876
Comments
Requiring a module means evaluating (running) it. That means it's being used from Node's perspective, which in turn means that...
...is not a reasonable expectation. The solution to your issue is in the Node.js ecosystem (get in touch with the maintainer, use another library, etc.), not in Node.js core.
That's the author-knows-best perspective. The alternative, user-centered perspective is that I as an end user want to be informed when a program uses experimental, deprecated or insecure APIs so I can evaluate what steps to take next. I think Node is doing the right thing here because the alternative is no end of libraries that swipe warnings under the carpet. |
I'll close this out in light of my previous comment. |
On the proposed solution:
This works on mac, but causes an infinite loop in linux as explained here: https://stackoverflow.com/questions/56374777/how-do-i-set-an-environment-variable-in-the-shebang-of-a-script-that-will-b You can try for yourself by running this on any ubuntu machine:
It never returns. As far as I am able to tell from having had multiple people try to solve this for a couple of weeks now, there is no solution. Back to the problem: I hear what you're saying about author-knows-best vs user-knows best, however I think there is an assumption here that users === Node.js developers, and that's not always true. I find this often in Node.js land. Ghost's perspective - where we build software in Node.js for non-developers - is often ignored or overlooked. Perhaps we're an edge-case, but then do I not get the option to work on or sponsor a solution, provided it doesn't create maintenance overhead or impact others? To further the discussion here, let's say I agree that require = use for now and let's also say that in the case of nested dependencies in Node.js land, user-knows-best is the correct approach. There's still two major parts to this bug report, which refers specifically to bin scripts, that I don't think this covers. I'll reiterate them here, but maybe I should raise them as two separate issues to further discuss their merit? 1. Users who are not Node.js developers. In my world, users are almost never Node.js developers. Our forum frequently sees issues where users start Ghost, see a bunch of warnings and panic. These users do not have any concept of Node.js other than as something they installed to run the software. You could argue that they should understand, but that's a privileged perspective, especially in the current climate - we're seeing more and more non-technical users getting involved because Ghost is a great way to start/monetize their business. With Ghost we have a CLI - a bin script - which we use to control how it is run, so that it's achievable by anyone who can use a command line. This tool exists for the purpose of hiding away complexity of Node.js from people who are not Node.js developers. This bin script should have the power to control its output. The users are not developers relying on dependencies. They are users of software who are actively put off by these warnings. 2. Random location of output The second nuance here is that in the context of a bin script I almost always want to output something. Put more strongly, a key purpose of a bin script is to control outputting something to the CLI. In this case, Node.js is injecting warnings that I did not ask for at some random, uncontrollable point in the output. This, above everything else, is what makes this issue so deeply problematic. I could - and intend to try to - use a method of rewriting stdout to workaround this issue, but the unpredictable output makes that much, much trickier to do. |
You don't have to rewrite stdout, did you miss https://nodejs.org/api/process.html#process_event_warning ? If you as app author know that warnings are being emitted, and don't want your users to see them (which is reasonable, you are the end consumer, its your call whether you are OK with the warnings), hook the event. |
@sam-github That works on ubuntu but not mac, see for yourself with Code here: ErisDS/node-warn-demo@9a6a5a5 |
I strongly suspect that what you are seeing is either different node versions, or that fs.promises isn't used on linux, there is nothing OS specific in the warning code paths. Anyhow, I misread the docs, sorry. The way to disable the default warning printer via code (not CLI or env config), is to simply |
Ok you're right that ubuntu isn't actually outputting the warning with v1 - not sure why as I don't see any OS specific codepaths in extract zip either. The code that triggers the warning is here. I can confirm though, that Gonna try pushing this out and hopefully it won't cause problems anywhere! Thanks for bearing with me and taking the time to find a solution. I really appreciate it ❤️ . Every issue/thread/stackoverflow I came cross was talking about either flags, env vars or stdout rewrites, so I'll try to update places with this info to make it more visible. |
closes #309, refs #307, refs nodejs/node#32876 - removes all the weird flags from the shebang which explode various OSes - instead use process.removeAllListeners() which should prevent warnings from being displayed using the node API and therefore in an OS-agnostic way
process.removeAllListeners('warning') |
Currently, there's a warning output by node about fs.promises being experimental. It's a warning in Node v10 that disappears in v12.
You don't have to use fs.promises to get this warning, it happens if one of your dependencies or sub-dependencies require it. In the case of bin scripts, the warning is shown to users of my script - this is what I mean by "bubbling up".
In my case, my code doesn't use this API, therefore the warning is not useful to me. Also in my case, the Node.js code that is outputting this warning is a bin script - specifically a CLI tool that performs validation. In this context the warning is not just useless, but actively problematic as it appears to form part of the validation output from the tool.
What steps will reproduce the bug?
I have created a minimal reproduction case here: https://github.com/ErisDS/node-warn-demo
To reproduce:
npm install -g node-warn-demo@v1
nvm use 10
(assuming you have nvm or similar to switch node versions)warn
You'll see the following output:
There are a few different subtle problems here:
How often does it reproduce? Is there a required condition?
This is always reproducible on Node v10.
What is the expected behavior?
I personally don't think it makes sense for the warning to bubble up from the dependency, certainly not on require.
Although I can see an argument that it's maybe useful knowledge for me as a user of the dependency, surely that should be up to the maintainer of the dependency whether or not they think their users need to know?
As the maintainer of the CLI tool, I definitely want control to prevent the warning bubbling up further as I know that it is of no use to my target audience, and is in fact going to be confusing for them.
What do you see instead?
There is no way to disable the warning.
We've tried adding the following to the top of the bin script:
#!/usr/bin/env node --no-warnings
- this works on mac and some linuxes but breaks on others as reported here#!/usr/bin/env -S node --no-warnings
- this works on mac and and some linuxes but breaks on ubuntu as reported hereI've seen other ideas about environment variables and so on, but every method I've tested either doesn't work at all, is not platform agnostic, or does not apply to the bin script context.
Additional information
You can see the real world example where we have this problem here on the gscan repo along with issues where we've had churn trying to fix it here and here.
The text was updated successfully, but these errors were encountered: