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

repl: show proxies as Proxy objects #16485

Closed
wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Oct 25, 2017

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Oct 25, 2017
@fhinkel
Copy link
Member

fhinkel commented Oct 25, 2017

Good idea!

@jasnell
Copy link
Member

jasnell commented Oct 25, 2017

I'm good with this but just noting that TC-39 has been consistent in their stance that the fact an object is a proxy should be transparent to users. This intentionally makes them non-transparent in the REPL... again, I'm perfectly fine with that and think it's a good thing to do.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

-1 from me, though I won't block.

In jsdom we are using Proxies for objects like element.attributes and document.querySelectorAll(). The [[ProxyTarget]] of those objects are in fact an empty object, and all the "properties" are made to look like they were actual properties on the object through a series of complex algorithms. If the original empty object was shown, it would convey 0 information to the users.

@bnoordhuis bnoordhuis closed this Nov 7, 2017
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Nov 7, 2017
Before this commit they transparently invoked their magic methods but
that sometimes throws confusing exceptions with misbehaving proxies.

This change is not wholly uncontroversial but we can always change the
default if necessary.  Let's see how it goes.

Fixes: nodejs#16483
PR-URL: nodejs#16485
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 7, 2017
@bnoordhuis
Copy link
Member Author

Since there is no real opposition I went ahead and landed this (commit 90a4390.)

I'll be conservative and tag it semver-major. If necessary we can flip the default to off. A similar line of reasoning applies to back-porting to the stable branches: could be done if needed, and default to off.

@BridgeAR
Copy link
Member

BridgeAR commented Dec 9, 2017

I am to tired to look properly into this but it seems like this introduced a regression on the repl in some cases. On my terminal if I open the repl and write e.g. util.inspect("test") it will from now on always print the color codes. This patch seems like a partial fix but it does not solve the issue completely.

-    self.writer = (obj) => util.inspect(obj, self.writer.options);
-    self.writer.options = Object.assign(writer.options, { colors: true });
+    self.writer = (obj) =>
+      util.inspect(obj, Object.assign(writer.options, { colors: true }));

@bnoordhuis
Copy link
Member Author

Ah, the bug is glaring in retrospect - the Object.assign() mutates util.inspect.defaultOptions if writer.options === util.inspect.defaultOptions. I'll fix it.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Dec 9, 2017
The `Object.assign()` calls introduced in commit 90a4390 ("repl: show
proxies as Proxy objects") mutated their first argument, causing the
`{ colors: true }` option from the REPL to leak over into the global
`util.inspect.defaultOptions` object.

Refs: nodejs#16485 (comment)
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 12, 2017
The `Object.assign()` calls introduced in commit 90a4390 ("repl: show
proxies as Proxy objects") mutated their first argument, causing the
`{ colors: true }` option from the REPL to leak over into the global
`util.inspect.defaultOptions` object.

Refs: nodejs#16485 (comment)

PR-URL: nodejs#17565
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@TimothyGu TimothyGu added the util Issues and PRs related to the built-in util module. label Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. 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
Development

Successfully merging this pull request may close these issues.

7 participants