-
Notifications
You must be signed in to change notification settings - Fork 29.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
lib: add option to silence warnings #36137
Changes from 1 commit
d9a04a9
22e6c16
a6959ae
ad00075
a8e4cef
7fed8b2
116efef
66e4a66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -175,11 +175,20 @@ function slowCases(enc) { | |||||
} | ||||||
} | ||||||
|
||||||
function emitExperimentalWarning(feature) { | ||||||
function emitExperimentalWarning(feature, code) { | ||||||
if (experimentalWarnings.has(feature)) return; | ||||||
const msg = `${feature} is an experimental feature. This feature could ` + | ||||||
'change at any time'; | ||||||
experimentalWarnings.add(feature); | ||||||
const { getOptionValue } = require('internal/options'); | ||||||
const opts = getOptionValue('--suppress-warnings'); | ||||||
const list = opts.split(','); | ||||||
let found = false; | ||||||
list.forEach((e) => { | ||||||
if (e === code) found = true; | ||||||
}); | ||||||
if (found) return; | ||||||
|
||||||
const msg = `[${code}] ${feature} is an experimental feature. ` + | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 Line 390 in 22293ea
function emitExperimentalWarning(feature) like this Line 188 in 51b4367
|
||||||
'This feature could change at any time'; | ||||||
process.emitWarning(msg, 'ExperimentalWarning'); | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
'use strict'; | ||
require('../common'); | ||
const tmpdir = require('../common/tmpdir'); | ||
const assert = require('assert'); | ||
const cp = require('child_process'); | ||
const fs = require('fs'); | ||
const path = require('path'); | ||
if (process.argv[2] === 'single-dep-skip') { | ||
new Buffer(10); | ||
} else if (process.argv[2] === 'multi-dep-skip') { | ||
new Buffer(10); | ||
tmpdir.refresh(); | ||
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt'); | ||
fs.rmdir(filePath, { recursive: true }, () => {}); | ||
} else if (process.argv[2] === 'single-exp-skip') { | ||
const vm = require('vm'); | ||
vm.measureMemory(); | ||
} else if (process.argv[2] === 'multi-exp-skip') { | ||
const vm = require('vm'); | ||
vm.measureMemory(); | ||
new AbortController(); | ||
} else { | ||
const child_single_dep_skip = cp.spawnSync(process.execPath, | ||
['--suppress-warnings=DEP0005', | ||
__filename, 'single-dep-skip']); | ||
assert.strictEqual(child_single_dep_skip.stdout.toString(), ''); | ||
assert.strictEqual(child_single_dep_skip.stderr.toString(), ''); | ||
const child_multi_dep_skip = cp.spawnSync(process.execPath, | ||
['--suppress-warnings=' + | ||
'DEP0005,DEP0147', | ||
__filename, | ||
'multi-dep-skip']); | ||
assert.strictEqual(child_multi_dep_skip.stdout.toString(), ''); | ||
assert.strictEqual(child_multi_dep_skip.stderr.toString(), ''); | ||
const child_single_exp_skip = cp.spawnSync(process.execPath, | ||
['--suppress-warnings=EXP0004', | ||
__filename, 'single-exp-skip']); | ||
assert.strictEqual(child_single_exp_skip.stdout.toString(), ''); | ||
assert.strictEqual(child_single_exp_skip.stderr.toString(), ''); | ||
const child_multi_exp_skip = cp.spawnSync(process.execPath, | ||
['--suppress-warnings=' + | ||
'EXP0004,EXP0001', | ||
__filename, | ||
'multi-exp-skip']); | ||
assert.strictEqual(child_multi_exp_skip.stdout.toString(), ''); | ||
assert.strictEqual(child_multi_exp_skip.stderr.toString(), ''); | ||
} |
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?
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.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.