FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: fix util.inspect with proxied function by starkwang · Pull Request #25244 · nodejs/node · GitHub

/ node Public

util: fix util.inspect with proxied function - #25244

Closed
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:util-proxy-function
ConversationCommits (1)ChecksFiles changed
Closed

util: fix util.inspect with proxied function#25244
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:util-proxy-function

Conversation

starkwang commented Dec 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #25212

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

nodejs-github-bot commented Dec 28, 2018

Copy link
Copy Markdown
Collaborator

@starkwang build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2083/pipeline

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 28, 2018

BridgeAR commented Dec 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

It does not hurt to do this but it will not solve the overall issue with proxies: they are difficult to reason with and even more difficult to write in a way that does not interfere with things.

We trigger multiple traps while inspecting an object and we can not work around all of the issues that might potentially come up.

Another example that would fail is using an empty array instead of the function.

bnoordhuis approved these changes Dec 28, 2018
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. I agree with Ruben's comment but this PR is still an improvement over the status quo so why not?

starkwang commented Dec 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

We trigger multiple traps while inspecting an object and we can not work around all of the issues that might potentially come up.

@BridgeAR Agreed. We can not fix all these issues. For example, an array with a weird proxy is hard to fix.

But at least we can make it a little better :-)

starkwang commented Dec 29, 2018

Copy link
Copy Markdown
Contributor Author

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

addaleax commented Dec 30, 2018

Copy link
Copy Markdown
Member

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 30, 2018

addaleax commented Dec 31, 2018

Copy link
Copy Markdown
Member

Landed in 6c7c77e

addaleax closed this Dec 31, 2018
addaleax pushed a commit that referenced this pull request Dec 31, 2018
util: fix util.inspect with proxied function
6c7c77e 
PR-URL: #25244
Fixes: #25212
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
util: fix util.inspect with proxied function
3797625 
PR-URL: #25244
Fixes: #25212
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
util: fix util.inspect with proxied function
c384144 
PR-URL: nodejs#25244
Fixes: nodejs#25212
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR mentioned this pull request Jan 16, 2019
Merged
MylesBorins mentioned this pull request Jan 24, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
util: fix util.inspect with proxied function
64847e3 
PR-URL: #25244
Fixes: #25212
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
util: fix util.inspect with proxied function
a9b6aea 
PR-URL: #25244
Fixes: #25212
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
util: fix util.inspect with proxied function
4e2ceba 
PR-URL: #25244
Fixes: #25212
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
cjihrig cjihrig approved these changes
JungMinu JungMinu approved these changes
+1 more reviewer
ryzokuken ryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Weird Proxy internal util.inspect error

8 participants

Back | FazBrowse Home | New Git URL