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.

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

util: inspect prototype properties if showHidden is truthy #30768

Closed
wants to merge 9 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 2, 2019

Please have a look at the commit messages for details.

I marked this as semver-major. We might also handle it as semver-minor instead but I wanted to be on the safe side. Please weight in case someone would like this to change.

I tried hard to implement this with the least performance overhead possible and I believe this is pretty much it.

Fixes: #30183

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

This makes sure that the regular expression matches all built-in
objects properly. So far a couple where missed.
This is only active if the `showHidden` option is truthy.

The implementation is a trade-off between accuracy and performance.
This will miss properties such as properties added to built-in data
types.

The goal is mainly to visualize prototype getters and setters such as:

class Foo {
  ownProperty = true
  get bar() {
    return 'Hello world!'
  }
}

const a = new Foo()

The `bar` property is a non-enumerable property on the prototype while
`ownProperty` will be set directly on the created instance.

The output is similar to the one of Chromium when inspecting objects
closer. The output from Firefox is difficult to compare, since it's
always a structured interactive output and was therefore not taken
into account.

Fixes: nodejs#30183
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 2, 2019
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 2, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I think we shouldn’t include prototype methods in the output.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 3, 2019

I think we shouldn’t include prototype methods in the output.

Originally I intended to filter those but I changed my mind while implementing the feature due to only inspecting user defined prototypes, not built-in ones. We do print all other functions while inspecting and this seemed more consistent. I personally would like to know all user defined methods when using showHidden (definitely not otherwise).

@nodejs/util PTAL and vote to get some further opinions:

❤️ filter prototype methods
🎉 keep user defined prototype methods

@addaleax
Copy link
Member

addaleax commented Dec 3, 2019

due to only inspecting user defined prototypes, not built-in ones

Isn’t that even worse, special-casing built-ins?

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 3, 2019

Isn’t that even worse, special-casing built-ins?

We already inspect some built-in prototype properties when using showHidden. This applies for TypedArrays for example.
If we'd inspect all prototype properties it would a) sometimes be very verbose and b) either require a huge amount of special handling or we'd have to reduce the amount of information we print by default because we currently inspect prototype getters (That would have to be removed and it would only be visible that there's a getter but not what value it currently corresponds too).

It would also mean a significant performance hit to inspect all prototypes and not only user defined ones. This implementation is quite cheap though and solves the feature request that I referenced.

@addaleax
Copy link
Member

addaleax commented Dec 3, 2019

@BridgeAR Right, I agree with everything you said – but what makes built-ins special? Doesn’t this feature get in the way for any larger classes, user-defined or not?

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 3, 2019

what makes built-ins special?

Users mostly know about built-in prototype properties (no matter if it's a method or another value) but they rarely know about user defined ones. Especially not, if it's not written by themselves.

Doesn’t this feature get in the way for any larger classes, user-defined or not?

Using showHidden definitely has the potential to be verbose. Knowing about user-defined properties seems useful though. Why would someone want to use showHidden in the first place if not with the intent to know (almost) everything about the input? Just built-in properties seem mostly redundant as I outlined above (we already inspect important ones).

I guess one way forward would be to allow a more fine-grained option. That way it's possible for the user to explicitly define what should be inspected:

showHidden: {
  prototypes: Boolean // Or even more fine grained as in: ['user', 'all', 'builtIn']
  weakEntries: Boolean,
  nonEnumerable: Boolean
}

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2019

@addaleax I decided to skip inspecting the methods for now. I might add more granular configuration later on where it's also possible to inspect methods. PTAL.

@nodejs-github-bot

This comment has been minimized.

doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
BridgeAR and others added 2 commits December 6, 2019 18:02
Co-Authored-By: Michaël Zasso <[email protected]>
Co-Authored-By: Michaël Zasso <[email protected]>
@nodejs-github-bot

This comment has been minimized.

@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 6, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@addaleax
Copy link
Member

@BridgeAR Removed my review here

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 13, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27627/ ✅ (yellow build with two frequent windows flakes)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2019
BridgeAR added a commit that referenced this pull request Dec 13, 2019
This makes sure that the regular expression matches all built-in
objects properly. So far a couple where missed.

PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR added a commit that referenced this pull request Dec 13, 2019
This is only active if the `showHidden` option is truthy.

The implementation is a trade-off between accuracy and performance.
This will miss properties such as properties added to built-in data
types.

The goal is mainly to visualize prototype getters and setters such as:

class Foo {
  ownProperty = true
  get bar() {
    return 'Hello world!'
  }
}

const a = new Foo()

The `bar` property is a non-enumerable property on the prototype while
`ownProperty` will be set directly on the created instance.

The output is similar to the one of Chromium when inspecting objects
closer. The output from Firefox is difficult to compare, since it's
always a structured interactive output and was therefore not taken
into account.

PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in 28ee032, fb8b483 🎉

@BridgeAR BridgeAR closed this Dec 13, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
This makes sure that the regular expression matches all built-in
objects properly. So far a couple where missed.

PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
This is only active if the `showHidden` option is truthy.

The implementation is a trade-off between accuracy and performance.
This will miss properties such as properties added to built-in data
types.

The goal is mainly to visualize prototype getters and setters such as:

class Foo {
  ownProperty = true
  get bar() {
    return 'Hello world!'
  }
}

const a = new Foo()

The `bar` property is a non-enumerable property on the prototype while
`ownProperty` will be set directly on the created instance.

The output is similar to the one of Chromium when inspecting objects
closer. The output from Firefox is difficult to compare, since it's
always a structured interactive output and was therefore not taken
into account.

PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
@targos targos added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Jan 14, 2020
@BridgeAR BridgeAR deleted the inspect-prototype-2 branch January 20, 2020 12:07
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 20, 2020
This makes sure that the regular expression matches all built-in
objects properly. So far a couple where missed.

PR-URL: nodejs#30768
Fixes: nodejs#30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 20, 2020
This is only active if the `showHidden` option is truthy.

The implementation is a trade-off between accuracy and performance.
This will miss properties such as properties added to built-in data
types.

The goal is mainly to visualize prototype getters and setters such as:

class Foo {
  ownProperty = true
  get bar() {
    return 'Hello world!'
  }
}

const a = new Foo()

The `bar` property is a non-enumerable property on the prototype while
`ownProperty` will be set directly on the created instance.

The output is similar to the one of Chromium when inspecting objects
closer. The output from Firefox is difficult to compare, since it's
always a structured interactive output and was therefore not taken
into account.

PR-URL: nodejs#30768
Fixes: nodejs#30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
This makes sure that the regular expression matches all built-in
objects properly. So far a couple where missed.

Backport-PR-URL: #31431
PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
This is only active if the `showHidden` option is truthy.

The implementation is a trade-off between accuracy and performance.
This will miss properties such as properties added to built-in data
types.

The goal is mainly to visualize prototype getters and setters such as:

class Foo {
  ownProperty = true
  get bar() {
    return 'Hello world!'
  }
}

const a = new Foo()

The `bar` property is a non-enumerable property on the prototype while
`ownProperty` will be set directly on the created instance.

The output is similar to the one of Chromium when inspecting objects
closer. The output from Firefox is difficult to compare, since it's
always a structured interactive output and was therefore not taken
into account.

Backport-PR-URL: #31431
PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins added backported-to-v12.x and removed backport-open-v12.x baking-for-lts PRs that need to wait before landing in a LTS release. labels Jan 30, 2020
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This makes sure that the regular expression matches all built-in
objects properly. So far a couple where missed.

Backport-PR-URL: #31431
PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This is only active if the `showHidden` option is truthy.

The implementation is a trade-off between accuracy and performance.
This will miss properties such as properties added to built-in data
types.

The goal is mainly to visualize prototype getters and setters such as:

class Foo {
  ownProperty = true
  get bar() {
    return 'Hello world!'
  }
}

const a = new Foo()

The `bar` property is a non-enumerable property on the prototype while
`ownProperty` will be set directly on the created instance.

The output is similar to the one of Chromium when inspecting objects
closer. The output from Firefox is difficult to compare, since it's
always a structured interactive output and was therefore not taken
into account.

Backport-PR-URL: #31431
PR-URL: #30768
Fixes: #30183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util.inspect getters ignored on class instance
6 participants