process: add CLI opt to hide experimental warnings #31000

codeman869 · 2019-12-17T00:48:13Z

Adds a command line option to supress experimental warnings. Currently
this cannot be accomplished without supressing all warnings (by using
the --no-warnings option). However, once this option is enabled, a user
can miss out on essential warnings as this supresses all warnings.
This commit adds the --no-experimental-warnings command line option to
allow users to ignore warnings they will expect while still being able
to monitor unexpected warnings.

Fixes: #30810

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

doc/api/cli.md

Trott · 2019-12-17T03:41:34Z

What's the use case for this? I want to make sure we're not allowing the suppression of warnings that we might not actually want to permit the suppression of.....

doc/api/cli.md

codeman869 · 2019-12-17T17:19:05Z

What's the use case for this? I want to make sure we're not allowing the suppression of warnings that we might not actually want to permit the suppression of.....

This is a really good question and I can see both sides as valid. First off, I think that the experimental warning feature is a good way to inform users about the stability of features they're using in their code and how quickly they can change. However, I can also see the argument about these warnings potentially being too numerous and could create clutter in error logs while a user is searching for an issue that is occurring in their code or debugging an unexpected condition. We've tried to reduce that as much as possible by not repeating experimental warnings and granted, they're probably not too difficult to ignore. But I think giving users an additional level of control to suppress these warnings while not suppressing all warnings provides some benefit in the long run as they can better monitor warnings which are more critical to them. Additionally, I think that since the user has to explicitly request to ignore these through the CLI, they will be knowledgeable with what they're doing and accidental misuse will be low.

jasnell · 2019-12-18T02:10:16Z

I'm not really in favor of this change right now. The warnings in this case serve a very important purpose.

addaleax · 2019-12-18T15:57:14Z

lib/internal/bootstrap/pre_execution.js

@@ -100,6 +100,8 @@ function patchProcessObject(expandArgv1) {
  addReadOnlyProcessAlias('_preload_modules', '--require');
  addReadOnlyProcessAlias('noDeprecation', '--no-deprecation');
  addReadOnlyProcessAlias('noProcessWarnings', '--no-warnings');
+  addReadOnlyProcessAlias('noExperimentalWarnings',
+                          '--no-experimental-warnings');


If this happens, please don’t use this. This is only for legacy flags that already have corresponding entries on the process object.

Thanks, good to know. I'll look into the code more for the correct place to add the flag.

@codeman869 You can look around for getOptionValue() in our source tree, that should help :)

jasnell

Just making the -1 explicit.

gireeshpunathil · 2019-12-20T09:30:20Z

IMO - given that experimental features are out for use, it is fair for users to expect to use those cleanly, or at least have some control over the prints. It may be unnecessary to force warnings every time, onto an informed user?

Qard

👎 from me. Using experimental features should be discouraged for normal use. Allowing the warnings to be silenced suggests that the warnings against using those features are not important.

cjihrig · 2019-12-22T23:58:03Z

I don't really have a strong opinion on whether or not this should land, but...

While I do think experimental warnings are important, I think explicitly opting out of them with a feature like this is a better option than, say, silencing all warnings with --no-warnings.

gireeshpunathil · 2019-12-23T04:36:45Z

@Qard -

Using experimental features should be discouraged for normal use.

But then we have been producing experimental features in release builds, including LTS all these while - I guess with an intent of folks using and experimenting those in the field?

Allowing the warnings to be silenced suggests that the warnings against using those features are not important.

May be at least for a subset of the users, who have seen the warning a few times, want to supress it because otherwise their scripts / monitors that process the stderr will have to be modified to ignore these warnings, and at a later point in time when a feature stabilizes they will have to again modify those scripts to remove those special code. With this feature in place, they don't have to do these special care.

My bottom line is: if we are looking at the field to gather usability insights to improve on experimental features, we also need to provide improved user experience with these features.

Qard · 2019-12-23T04:47:01Z

In my opinion, the only reason they are shipped behind runtime flags on release builds rather than behind build flags is because it increases the likelihood people will actually try the new things out and encounter the bugs so we can be more certain of stability before giving it a proper release. If you had to build your own custom Node.js just to try a new feature I suspect out "release" features would be a lot less stable due to less user testing.

devsnek · 2019-12-23T04:57:12Z

i don't think experimental warnings should be suppressible. when experimental features are enabled, node is running in an unstable state, and that should be made very clear to whoever is running it. (maybe we should whitelist them from --no-warnings)

legendecas · 2019-12-23T05:15:11Z

Allow list SGTM.

Adds a command line option to supress experimental warnings. Currently this cannot be accomplished without supressing all warnings (by using the --no-warnings option). However, once this option is enabled, a user can miss out on essential warnings as this supresses all warnings. This commit adds the --no-experimental-warnings command line option to allow users to ignore warnings they will expect while still being able to monitor unexpected warnings. Fixes: nodejs#30810

addaleax

LGTM, this is a good idea 👍

antsmartian

Nice idea, LGTM

gireeshpunathil · 2019-12-24T05:42:19Z

@devsnek -

i don't think experimental warnings shouldn't be suppressible.

looks like the double negative statement (which translates to experimental warnings should be suppressible) is NOT what you meant?

when experimental features are enabled, node is running in an unstable state, and that should be made very clear to whoever is running it.

One can bring in a counter argument that: on the same ground, --no-warnings should be removed as well?

There is no disagreement about the importance of the experimental warnings. The question is about its consumability - when message are produced every run of the workload without a bailout option, one can argue that users are being punished for using it.

We have evidence of experimental features being used in production (async_hooks , diagnostic report etc.) despite their stability status, owing to their unique capabilities. Implementing this option will help Node.js users to run their production workloads with less clutter.

codeman869 · 2020-01-06T19:06:27Z

Since this seems to be a controversial change, do we want to have TSC evaluate for a final decision to either move forward or close this PR and related issue/feature request?

jasnell · 2020-01-08T15:37:34Z

src/node_options.cc

+AddOption("--no-experimental-warnings",
+            "silence all experimental process warnings",
+            &EnvironmentOptions::no_experimental_warnings,
+            kAllowedInEnvironment);


If this did move forward, it should not be permitted within NODE_OPTIONS

jasnell · 2020-01-08T15:44:38Z

I would be ok with moving forward on this if it were a mechanism to disable specific identified warnings rather than an entire category. Specifically, we have the ability to attach identifiers to warnings that we currently use for deprecations (e.g. the DEP00XX identifiers) and can also use for experimental warnings (e.g. a hypothetical EXP00XX identifier). We could then disable specific warnings by id using the command-line flag (e.g. node --no-warnings=EXP0001,DEP0022) to disable only specific individual warnings.

github-actions · 2020-10-19T18:04:47Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions · 2020-11-19T00:03:01Z

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

gireeshpunathil · 2020-11-19T02:43:51Z

2 PRs on it, re-opening

jwarkentin · 2022-02-08T05:11:35Z

So what exactly is holding this up at the moment? What work needs to be done to close it out? Not sure if I can be of help or not but I'm having to disable all warnings with --no-warnings at the moment, which I really don't like doing. I figured it was obvious I had made an informed decision to use an experimental feature since I have to explicitly invoke node with --experimental-vm-modules, so getting blasted with the same warning telling me that repeatedly while testing code just needlessly clutters and obfuscates the output. I'm a lot more worried about broadly disabling all warnings just to make my output readable than I am about using an experimental feature that I'm well-aware I'm using.

Trott · 2022-02-08T19:21:43Z

So what exactly is holding this up at the moment?

I think someone needs to put in the work to make this so that the user chooses which experimental warnings to hide.

@jasnell @Qard You're the ones with the request-for-changes. Can you confirm or clarify?

Qard · 2022-02-08T23:08:37Z

My concern is that many people will just always run with that flag and ignore experimental warnings entirely. Even worse would be if test runners and the like start using it by default, creating an environment with different behaviour from what is typical.

Experimental features should not be used in a production app. We're just further enabling people to ignore our judgement about what is and is not ready for production use. If you're worried about log pollution it's because you're using something in prod that you should not be. If a thing is really not actually experimental anymore we should be advocating for marking that thing as stable, not for allowing experimental warnings to be ignored.

I feel we need to be very intentional with our messaging around experimental features that they are not safe to use in production and could change or disappear at any time. The presence of experimental features in a release is intended solely for experimentation--we want people to try out an uncertain idea and provide feedback so we can iterate and move it towards stable. When it is actually ready for people to use in prod we will mark it stable. Maybe we need another stage between experimental and stable to indicate something that's no longer in a could-break-at-any-time state but not quite yet at a point where we want to make firm promises about the stability?

jwarkentin · 2022-02-15T01:30:37Z

Maybe we need another stage between experimental and stable to indicate something that's no longer in a could-break-at-any-time state but not quite yet at a point where we want to make firm promises about the stability?

I would be good with anything that adds meaningful clarity.

For any robust production setup, the messages aren't an issue, IMO. It's easy to ship logs off to a log management system and filter through them as needed. The issue is spamming of the console in development. At the same time, I think that's really beside the point.

The code I'm working on right now isn't high stakes and it isn't in production yet. The experimental features are only required for the test environment, anyway. The production code doesn't use anything experimental. I've spent plenty of time digging through the code and related issues for Node and many libs, just to get an experimental environment working and to make sure I understand the limitations and potential problems. That's all to say, I know what I'm doing. As someone that knows what I'm doing, has a use for the experimental feature, is happy to test the experimental feature and provide feedback, and has already knowingly & explicitly enabled the feature, I would like the option to not have my console spammed with messages telling me what I've done.

Also, I don't think the console spamming is what's stopping test runners from relying on experimental features. They're already in a position to easily filter out that noise if desired. In reality, it would incur too much risk to support. It would be insane for any remotely popular project which is exactly why it isn't already being done.

jasnell · 2022-02-18T22:31:39Z

I maintain that my preference is for an option to disable individual experimental warnings by code/id as opposed to all warnings, and for that not to be permitted on NODE_OPTIONS.

jwarkentin · 2022-02-19T20:55:57Z

I very much agree. That would be ideal. I have no idea how to implement that myself or I would. It sounds a bit more involved than a naive, if less ideal, solution. Would it be worth an initial quick, blunt mechanism before iterating to the more nuanced flags?

TomasHubelbauer · 2022-04-16T09:40:24Z

Not permitting disabling experimental warnings and not permitting disabling experimental warnings in NODE_OPTIONS just means people will use --no-warnings, which is already permissible in NODE_OPTIONS. People can make an informed decision when opting to use an experimental warning, even globally through NODE_OPTIONS and IMO should be allowed to own this decision without having a non-dismissable banner litter the output of their every Node invocation. I believe not allowing to disable experimental warnings in NODE_OPTIONS would do more harm than allowing it due to this.

ghost · 2022-05-14T05:26:35Z

Maybe we need another stage between experimental and stable to indicate something that's no longer in a could-break-at-any-time state but not quite yet at a point where we want to make firm promises about the stability?

I would be good with anything that adds meaningful clarity.

For any robust production setup, the messages aren't an issue, IMO. It's easy to ship logs off to a log management system and filter through them as needed. The issue is spamming of the console in development. At the same time, I think that's really beside the point.

The code I'm working on right now isn't high stakes and it isn't in production yet. The experimental features are only required for the test environment, anyway. The production code doesn't use anything experimental. I've spent plenty of time digging through the code and related issues for Node and many libs, just to get an experimental environment working and to make sure I understand the limitations and potential problems. That's all to say, I know what I'm doing. As someone that knows what I'm doing, has a use for the experimental feature, is happy to test the experimental feature and provide feedback, and has already knowingly & explicitly enabled the feature, I would like the option to not have my console spammed with messages telling me what I've done.

Also, I don't think the console spamming is what's stopping test runners from relying on experimental features. They're already in a position to easily filter out that noise if desired. In reality, it would incur too much risk to support. It would be insane for any remotely popular project which is exactly why it isn't already being done.

Same here, Jest kicks of as many processes as there are test files (as possible.) Therefore the spamming is real.

broofa · 2022-05-16T17:14:25Z

Same here, Jest kicks of as many processes as there are test files (as possible.) Therefore the spamming is real.

Ditto. Using ava and every test file logs this warning:

(node:20159) ExperimentalWarning: Custom ESM Loaders is an experimental feature.
This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

Landed on this PR after seeing this several 1,000 times and hoping there was a simple way to suppress the warning (because I'm using esbuild-node-loader to avoid a separate TS compilation step.)

I would be ok with moving forward on this if it were a mechanism to disable specific identified warnings rather than an entire category.

💯 agree. I would much prefer not being forced into an all-or-nothing situation on this.

FWIW, in the above case I'd expect to fix the issue with something like --ignore-warning=20159.

mhdawson · 2022-05-25T21:43:13Z

We discussed in the TSC meeting today (note we only had 6 people so a smallish subset). From the discussion there would be no objection from those who were there if there was an option to disabled specific experimental warnings and that was also supported in NODE_OPTIONS.

BridgeAR · 2022-05-25T22:38:12Z

@medikoo

In tests sometimes I mock modules with variants that expose a subset of properties. I also tweak modules cache (using just publicly documented API), so new require's point exports mocks and not original exports.

Having such setup, with Node.js v14 I started to observe significant warnings pollution due to that

The warnings should vanish in case the modules contain a .loaded property set to true. Do you mind trying that out?

The cache is public and that can not change anymore. It has also been publicly documented from very early on. It's existance is documented but not really how to add new entries. This is something that would be benefitial. We could in theory detect properties being added to the cache and ignore those from the circularity check. The implementation of such check would not be trivial though (using proxies with a complicated behavior).

medikoo · 2022-05-26T10:40:48Z

@BridgeAR great thanks for that hint. Interestingly I do not see those warnings anymore, but also a lot of has changed in this code area since my last comment.

If I'll see the warnings again, I'll check if your solution helps

Warnings are no longer observed without this util so it appears as obsolete BREAKING CHANGE: `preventCicrularDepPropertyWarning` util was removed as no longer needed. If issue surfaces again, it'll be great to see if solution proposed in following comment helps: nodejs/node#31000 (comment)

mcollina · 2022-06-01T12:26:00Z

Closing this in favor of #40940, being able to skip individual warnings is going to be less contentious.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Dec 17, 2019

richardlau reviewed Dec 17, 2019

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

cjihrig reviewed Dec 17, 2019

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

codeman869 force-pushed the feat/no-exp-warnings branch from 654fce4 to 5bf1ba3 Compare December 17, 2019 17:25

addaleax reviewed Dec 18, 2019

View reviewed changes

jasnell requested changes Dec 18, 2019

View reviewed changes

Qard requested changes Dec 22, 2019

View reviewed changes

codeman869 force-pushed the feat/no-exp-warnings branch from 5bf1ba3 to 7d73b67 Compare December 23, 2019 16:27

codeman869 force-pushed the feat/no-exp-warnings branch from 7d73b67 to 0693a79 Compare December 23, 2019 17:18

addaleax approved these changes Dec 23, 2019

View reviewed changes

antsmartian approved these changes Dec 24, 2019

View reviewed changes

gireeshpunathil mentioned this pull request Dec 24, 2019

A flag for disabling specific warnings #27355

Closed

BridgeAR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 6, 2020

Trott mentioned this pull request Jan 6, 2020

Node.js Foundation Technical Steering Committee (TSC) Meeting 2020-01-08 nodejs/TSC#796

Closed

jasnell reviewed Jan 8, 2020

View reviewed changes

mhdawson mentioned this pull request Jan 13, 2020

Node.js Foundation Technical Steering Committee (TSC) Meeting 2020-01-15 nodejs/TSC#801

Closed

cjihrig mentioned this pull request Nov 16, 2020

lib: add option to silence warnings #36137

Closed

4 tasks

github-actions bot closed this Nov 19, 2020

gireeshpunathil reopened this Nov 19, 2020

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

tniessen mentioned this pull request Nov 23, 2021

Suppress specific warnings #40940

Closed

mhdawson added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label May 16, 2022

mhdawson mentioned this pull request May 24, 2022

Node.js Technical Steering Committee (TSC) Meeting 2022-05-25 nodejs/TSC#1230

Closed

medikoo mentioned this pull request May 26, 2022

Remove preventCicrularDepPropertyWarning util serverless/test#141

Merged

mhdawson mentioned this pull request May 30, 2022

Node.js Technical Steering Committee (TSC) Meeting 2022-06-01 nodejs/TSC#1234

Closed

mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jun 1, 2022

mcollina closed this Jun 1, 2022

Ethan-Arrowood mentioned this pull request Nov 10, 2023

src: add --disable-warning option #50661

Merged

4 tasks

process: add CLI opt to hide experimental warnings #31000

process: add CLI opt to hide experimental warnings #31000

Conversation

codeman869 commented Dec 17, 2019

Checklist

Trott commented Dec 17, 2019

codeman869 commented Dec 17, 2019

jasnell commented Dec 18, 2019

addaleax Dec 18, 2019

Choose a reason for hiding this comment

codeman869 Dec 19, 2019

Choose a reason for hiding this comment

addaleax Dec 19, 2019

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 20, 2019

Qard left a comment

Choose a reason for hiding this comment

cjihrig commented Dec 22, 2019

gireeshpunathil commented Dec 23, 2019

Qard commented Dec 23, 2019

devsnek commented Dec 23, 2019 • edited Loading

legendecas commented Dec 23, 2019

addaleax left a comment

Choose a reason for hiding this comment

antsmartian left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 24, 2019

codeman869 commented Jan 6, 2020

jasnell Jan 8, 2020

Choose a reason for hiding this comment

jasnell commented Jan 8, 2020

github-actions bot commented Oct 19, 2020

github-actions bot commented Nov 19, 2020

gireeshpunathil commented Nov 19, 2020

jwarkentin commented Feb 8, 2022

Trott commented Feb 8, 2022

Qard commented Feb 8, 2022

jwarkentin commented Feb 15, 2022

jasnell commented Feb 18, 2022

jwarkentin commented Feb 19, 2022

TomasHubelbauer commented Apr 16, 2022

ghost commented May 14, 2022

broofa commented May 16, 2022

mhdawson commented May 25, 2022 • edited Loading

BridgeAR commented May 25, 2022

medikoo commented May 26, 2022

mcollina commented Jun 1, 2022

devsnek commented Dec 23, 2019 •

edited

Loading

mhdawson commented May 25, 2022 •

edited

Loading