Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
lib: add option to silence warnings#36137
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
PoojaDurgad commented Nov 16, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
benjamingr commented Nov 16, 2020
Why? |
cjihrig commented Nov 16, 2020
benjamingr commented Nov 16, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@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 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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mscdex commented Nov 16, 2020
It looks like this option doesn't allow specifying multiple features, is this intentional? |
a13be9c to e1123e3ComparePoojaDurgad commented Nov 18, 2020
I'm working to address the issues. |
jasnell left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this 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,DEP002Or, by warning type:
$ node --no-warnings=ExperimentalWarning,DeprecationWarningNote: 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.
576772c to ccef73cComparePoojaDurgad commented Nov 20, 2020
@mscdex , @jasnell , @benjamingr - I pushed some changes . please have a look.
|
Introduce an option `--suppress-warnings` to silence experimental and deprecation warnings for specified features Fixes: nodejs#30810 Co-Authored-By: Cody Deckard <[email protected]>
ccef73c to d9a04a9Comparedoc/api/cli.md Outdated
| added: REPLACEME | ||
| --> | ||
| Silence deprecation and experimental warnings for specified codes. |
There was a problem hiding this comment.
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?
PoojaDurgadDec 30, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
PoojaDurgadJan 1, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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; | ||
| constmsg=`[${code}] ${feature} is an experimental feature. `+ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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');
Line 390 in 22293ea
| emitExperimentalWarning('vm.measureMemory'); |
function emitExperimentalWarning(feature) like this Line 188 in 51b4367
| functionemitExperimentalWarning(feature){ |
martinmunillas commented Oct 24, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@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 commented Feb 27, 2023
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 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 |
Introduce an option
--suppress-warningsto silence experimental and deprecation warnings for specified featuresFixes: #30810
Co-Authored-By: Cody Deckard [email protected]
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes