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

lib: add option to silence warnings #36137

Closed
wants to merge 8 commits into from

Conversation

PoojaDurgad
Copy link
Contributor

@PoojaDurgad PoojaDurgad commented Nov 16, 2020

Introduce an option --suppress-warnings to silence experimental and deprecation warnings for specified features

Fixes: #30810

Co-Authored-By: Cody Deckard cjdeckard@gmail.com

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

@nodejs-github-bot 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 Nov 16, 2020
@benjamingr
Copy link
Member

Why?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2020

Why?

There is prior discussion in #30810 and #31000.

@benjamingr
Copy link
Member

benjamingr commented Nov 16, 2020

@cjihrig in that case this PR goes against the TSC consensus discussed in the PR?

Edit: Oh I see this allows disabling the warning for a specific feature and not all features.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2020

I haven't reviewed this PR yet, but IIRC, the idea was that we could support silencing specific warnings, not just experimental warnings.

lib/internal/util.js Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Nov 16, 2020

It looks like this option doesn't allow specifying multiple features, is this intentional?

@PoojaDurgad
Copy link
Contributor Author

I'm working to address the issues.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any process.emitWarning() may include a code option.

Rather than introducing a new cli option just to disable experimental warnings, we should extend the existing --no-warnings to allow silencing specific codes:

$ node --no-warnings=EXP001,DEP002

Or, by warning type:

$ node --no-warnings=ExperimentalWarning,DeprecationWarning

Note: We do not currently assign individual codes to experimental warnings. We should start doing so.

Additionally, the way this PR makes the change, experimental warnings would not be emitted to the process.on('warning') event, which is a mistake. The existing --no-warnings flag will only suppress the default action of printing warnings to stderr but those will still be emitted to process.on('warning'). This is intentional behavior that should be preserved here as well.

@PoojaDurgad PoojaDurgad force-pushed the exp-warning branch 2 times, most recently from 576772c to ccef73c Compare November 20, 2020 08:00
@PoojaDurgad PoojaDurgad changed the title Add an option to silence experimental-warnings lib: add option to silence warnings Nov 20, 2020
@PoojaDurgad
Copy link
Contributor Author

@mscdex , @jasnell , @benjamingr - I pushed some changes . please have a look.

  • Introduced new flag --suppress-warnings
  • Introduced codes for experimental warnings.
  • Enabled the flag to take multiple codes.
  • Adjusted tests to the changes.
  • Added a test.

Introduce an option `--suppress-warnings`
to silence experimental and deprecation
warnings for specified features

Fixes: nodejs#30810

Co-Authored-By: Cody Deckard <cjdeckard@gmail.com>
doc/api/cli.md Outdated
added: REPLACEME
-->

Silence deprecation and experimental warnings for specified codes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between this and the existing --no-warnings flag?

Copy link
Contributor Author

@PoojaDurgad PoojaDurgad Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell - I have introduced the new flag --suppress-warnings and also introduced codes for experimental warnings which enables this flag to silence the experimental and deprecation warnings for supplied codes. The idea is to support silencing warnings for experimental and deprecation warnings and enabled the flag to take multiple codes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and what I'm saying is that the --no-warnings flag can just be extended to support this without introducing a new flag. --no-warnings (with no arguments) would continue to work as it does today, while --no-warnings={list of codes} would suppress the named codes. In either case, the documentation here should indicate that the flag takes a list of codes.

Copy link
Contributor Author

@PoojaDurgad PoojaDurgad Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell - well instead of introducing new cli option we can extend the existing --no-warnings option right . yeah I understood and thanks for the review.

});
if (found) return;

const msg = `[${code}] ${feature} is an experimental feature. ` +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right way to include the code. The emitWarning api already has an option for including the code correctly. Also, I would add this check into the emitWarning processing so it can be applied generically to all warning types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this comment was marked resolved as the issue is still there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without knowing, it was marked as resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell - I have a doubt , can you confirm one thing please , the experimental warning vm.measureMemory API calls emitExperimentalWarning like this emitExperimentalWarning('vm.measureMemory');

node/lib/vm.js

Line 390 in 22293ea

emitExperimentalWarning('vm.measureMemory');
so the function takes a string and it matches with the function emitExperimentalWarning(feature) like this
function emitExperimentalWarning(feature) {
so if I'm not wrong this call does not include an option for codes. If the above file modification is not the right way to include the code, then how to do that? can you suggest me.

@PoojaDurgad PoojaDurgad marked this pull request as draft February 4, 2021 05:37
@martinmunillas
Copy link

martinmunillas commented Oct 24, 2021

@PoojaDurgad are you willing to keep working on this?, I ask because is something I'm interested in too and i would be glad to help

@bnoordhuis
Copy link
Member

I'm going to close this because the pull request has a great many conflicts and because the requested feature from #30810 has been implemented but thanks anway, @PoojaDurgad, it's much appreciated.

@haidaqn
Copy link

haidaqn commented Oct 18, 2023

I am getting this error when integrating nextjs 13 with clerk and using node 16.13.0 : " ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
(Use node --trace-warnings ... to show where the warning was created) "

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to suppress warnings by type (or just experimental warnings)
10 participants