process: refactor emitWarning #20726
Conversation
nodejs-github-bot
added
the
process
lib/internal/process/warning.js
Outdated
| @@ -116,24 +116,23 @@ function setupProcessWarnings() { | |||
| ctor = type; | |||
| code = undefined; | |||
| type = 'Warning'; | |||
| } else if (type !== undefined && typeof type !== 'string') { | |||
There was a problem hiding this comment.
I don't think this is the same. If type is an object but type.type is truthy and not a string then this won't throw anymore.
| throw new ERR_INVALID_ARG_TYPE('type', 'string', type); | ||
| if (warning === undefined || typeof warning === 'string') { | ||
| } | ||
| if (typeof warning === 'string') { |
There was a problem hiding this comment.
This removes the possibility of an undefined warning that has the other properties specified.
There was a problem hiding this comment.
That is not officially supported.
There was a problem hiding this comment.
While not documented, as far as I can tell this was requested in #4782
It has also worked this way for two years... I don't know what usage people have for this or what they're passing to it. It might very well be a breaking change.
There was a problem hiding this comment.
I agree with @apapirovski on this. This is a breaking change even if not properly documented.
There was a problem hiding this comment.
process.emitWarning() produces: (node:XXXX) Warning. I am not convinced that this is useful at all. This seems like a bugfix as no args should result in an error. Therefore I would say it is a patch. However, I would also keep this if it makes this semver-major.
Ping @nodejs/tsc please also give an opinion.
There was a problem hiding this comment.
process.emitWarning()produces:(node:XXXX) Warning. I am not convinced that this is useful at all.
process.emitWarning(undefined, 'fhqwhgads', 'come-on') produces the slightly more useful (node:91812) [come-on] fhqwhgads.
I have no idea if anyone uses that pattern or not. Maybe it's time for me to figure out how to use Gzemnid.
In the spirit of the onboarding-extras.md:
be conservative – that is, if a change has the remote chance of breaking something, go for semver-major
It seems like this should be semver-major as it has a "remote change of breaking something".
(If we're not going to call this semver-major, I won't push back, but we really ought to remove the quoted sentence above from the doc since it is clearly not something we abide by in that case.)
BridgeAR
added
the
semver-major
|
I marked it as semver-major. PTAL. |
|
I would expect to see more tests changed as it's semver-major. Can you add some more test that verify the change in behavior? |
BridgeAR
force-pushed
the
improve-warnings
branch
from
869ffcf
to
f42b5d4
Compare
|
Rebased due to conflicts. I also added a test case for the changed behavior as requested. |
BridgeAR
added
the
author ready
|
I agree with @apapirovski. This PR was "marketed" as a simple refactoring by its title… and it should stay this way at least for this PR, or we could change the title to make it clear that we are changing the behavior. |
BridgeAR
changed the title
Remove a couple of obsolete checks by refactoring the code with else. Also remove the possibility of an undefined warning. That is neither documented nor does it result in a usable warning.
BridgeAR
force-pushed
the
improve-warnings
branch
from
f42b5d4
to
10562c2
Compare
|
@TimothyGu @apapirovski I changed the description of the PR accordingly. |
Remove a couple of obsolete checks by refactoring the code with else. Also remove the possibility of an undefined warning. That is neither documented nor does it result in a usable warning. PR-URL: nodejs#20726 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
CI https://ci.nodejs.org/job/node-test-pull-request/15892/ (passed) Landed in 49681e7 🎉 |
Currently the code does a couple of checks to often even though they
are not necessary.
Also remove a double whitespace in a error comment.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes