console: fix console.dir crash on a revoked proxy#43100
console: fix console.dir crash on a revoked proxy#43100nodejs-github-bot merged 4 commits intonodejs:masterfrom
Conversation
cdb0ea5 to
bb9c1e7
Compare
Fixes: nodejs#43095 Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
This commit removes extra spaces looking unnecessary if the `joinedOutput` of type `Array` is empty on `reduceToSingleString`. e.g) Proxy [ ] -> Proxy [] Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
bb9c1e7 to
5723aaf
Compare
There was a problem hiding this comment.
Thanks for the PR! I am going to ignore the implementation details for now to clarify the visualization first:
I am wondering how we want to visualize the revoked proxy with showProxy === false. It's not null but we do not know about the value either. We could just print Revoked Proxy [] or Proxy [ null, null ]. This would however expose the information that it's a proxy which is a hidden fact for not-revoked proxies.
The situation for showProxy === true is simpler: just either use Revoked Proxy [] or Proxy [ null, null ]. I personally like the Revoked Proxy [] a bit better. Any other opinions @nodejs/util?
|
Thanks for starting the clarification! FAYI, this PR's current visualization is the following: let r = Proxy.revocable({}, {});
console.log(1, inspect(r.proxy));
console.log(2, inspect(r.proxy, { showProxy: true }));
r.revoke();
console.log(3, inspect(r.proxy));
console.log(4, inspect(r.proxy, { showProxy: true }));1 {}
2 Proxy [ {}, {} ]
3 Proxy []
4 Proxy [ null, null ](3) was the part I was most confused of. |
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
ddf91e4 to
161dbe5
Compare
|
@daeyeon what do you think about: 3 + 4 using |
There was a problem hiding this comment.
It is possible to simplify the code by using this diff (line numbers must be ignored as they are not identical), if we do not care to distinguish visualization with the showProxy option:
@@ -759,6 +758,9 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
// any proxy handlers.
const proxy = getProxyDetails(value, !!ctx.showProxy);
if (proxy !== undefined) {
+ if (proxy === null) {
+ return ctx.stylize('<Revoked Proxy>', 'special');
+ }
if (ctx.showProxy) {
return formatProxy(ctx, proxy, recurseTimes);
}
@@ -1967,6 +1960,9 @@ function hasBuiltInToString(value) {
const getFullProxy = false;
const proxyTarget = getProxyDetails(value, getFullProxy);
if (proxyTarget !== undefined) {
+ if (proxyTarget === null) {
+ return true;
+ }
value = proxyTarget;
}The last change moves the extra check to only be executed for proxies and no other entries (value?.toString has to run the check for all entries).
87a63f5 to
a19e077
Compare
|
@BridgeAR Thanks for the review. I also think that
Also fixed. |
|
LGTM! |
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
a19e077 to
13964d0
Compare
|
Landed in f7cd3f6 |
Fixes: nodejs/node#43095 Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: nodejs/node#43100 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Fixes: #43095
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com