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.

Sign up for GitHub

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

Jump to bottom

process: refactor emitWarning #20726

Closed
wants to merge 1 commit into from
Closed

process: refactor emitWarning #20726

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

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), 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 the process Issues and PRs related to the process subsystem. label May 14, 2018
@BridgeAR
Copy link
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/14870/

apapirovski
apapirovski reviewed May 14, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

There are changes here that change how the code functions. I'm -1 on those happening. What would be left after reverting those seems fine to me.

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') {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

lib/internal/process/warning.js
throw new ERR_INVALID_ARG_TYPE('type', 'string', type);
if (warning === undefined || typeof warning === 'string') {
}
if (typeof warning === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

This removes the possibility of an undefined warning that has the other properties specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is not officially supported.

Copy link
Member

@apapirovski apapirovski May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @apapirovski on this. This is a breaking change even if not properly documented.

Copy link
Member Author

@BridgeAR BridgeAR May 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 18, 2018
@BridgeAR
Copy link
Member Author

I marked it as semver-major. PTAL.

@mcollina
Copy link
Member

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?

mcollina
mcollina approved these changes Jun 2, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR requested a review from a team June 11, 2018 01:17
@BridgeAR
Copy link
Member Author

Rebased due to conflicts. I also added a test case for the changed behavior as requested.

CI https://ci.nodejs.org/job/node-test-pull-request/15379/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 11, 2018
@TimothyGu
Copy link
Member

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 BridgeAR changed the title process: reduce possible code paths by using else process: refactor emitWarning Jun 25, 2018
@BridgeAR
process: refactor emitWarning
10562c2 
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
Copy link
Member Author

@TimothyGu @apapirovski I changed the description of the PR accordingly.

CI https://ci.nodejs.org/job/node-test-pull-request/15591/

mcollina
mcollina approved these changes Jun 25, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Still LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 16, 2018
@BridgeAR
process: refactor emitWarning
49681e7 
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>
@BridgeAR
Copy link
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/15892/ (passed)

Landed in 49681e7 🎉

@BridgeAR BridgeAR closed this Jul 16, 2018
@BridgeAR BridgeAR deleted the improve-warnings branch January 20, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@apapirovski apapirovski apapirovski left review comments

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @mcollina @TimothyGu @apapirovski @jasnell @Trott @nodejs-github-bot