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

util: use constructor name #14886

Closed
wants to merge 1 commit into from
Closed

util: use constructor name #14886

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

For circular references util.inspect always prints Array
or Object not matter if it is a subclass or not.

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
Affected core subsystem(s)

util

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 17, 2017
BridgeAR
BridgeAR commented Aug 17, 2017
View reviewed changes
lib/util.js Outdated
} else if (Array.isArray(value)) {
if (constructor)
return ctx.stylize(`[${constructor.name}]`, 'special');
if (Array.isArray(value))
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this check is redundant as soon as #14881 has landed. Right now Arrays set the constructor to null and the check has to be done again.

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 17, 2017
@TimothyGu
Copy link
Member

Commit message nit: doesn't this apply to objects that exceed the depth limit rather than circular objects? Or both?

@BridgeAR
Copy link
Member Author

@TimothyGu you are absolutely right! Fixed

@BridgeAR
Copy link
Member Author

PTAL I changed the commit message and this could land otherwise.

@hiroppy
Copy link
Member

hiroppy commented Aug 26, 2017

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

@BridgeAR
Copy link
Member Author

I would like to land this after #14881 as that one would otherwise not cleanly land on 8.x. Otherwise I would have already landed that. Marking this as blocked because of that.

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Aug 26, 2017
@BridgeAR BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Sep 15, 2017
@BridgeAR
util: use constructor name
572f1ef 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.
@BridgeAR
Copy link
Member Author

Rebased. PTAL, I would like to land this very soon.

@BridgeAR
Copy link
Member Author

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

@BridgeAR
Copy link
Member Author

Landed in b1c8f15

@BridgeAR BridgeAR closed this Sep 19, 2017
BridgeAR added a commit that referenced this pull request Sep 19, 2017
@BridgeAR
util: use constructor name
b1c8f15 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.

PR-URL: #14886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
util: use constructor name
e1e9eb1 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.

PR-URL: nodejs/node#14886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
util: use constructor name
f2844dc 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.

PR-URL: nodejs/node#14886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@BridgeAR BridgeAR deleted the add-proper-constructor branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @TimothyGu @hiroppy @refack @jasnell @cjihrig @nodejs-github-bot