refactor(ext/node): Use Deno.inspect #17960
Conversation
| // Provide a hook for user-specified inspect functions. | ||
| // Check that value is an object with an inspect function on it. | ||
| if (ctx.customInspect) { | ||
| const maybeCustom = value[customInspectSymbol]; | ||
| if ( | ||
| typeof maybeCustom === "function" && | ||
| // Filter out the util module, its inspect function is special. | ||
| maybeCustom !== inspect && | ||
| // Also filter out any prototype objects using the circular check. | ||
| !(value.constructor && value.constructor.prototype === value) | ||
| ) { | ||
| // This makes sure the recurseTimes are reported as before while using | ||
| // a counter internally. | ||
| const depth = ctx.depth === null ? null : ctx.depth - recurseTimes; | ||
| const isCrossContext = proxy !== undefined || | ||
| !(context instanceof Object); | ||
| const ret = maybeCustom.call( | ||
| context, | ||
| depth, | ||
| getUserOptions(ctx, isCrossContext), | ||
| ); | ||
| // If the custom inspection method returned `this`, don't go into | ||
| // infinite recursion. | ||
| if (ret !== context) { | ||
| if (typeof ret !== "string") { | ||
| return formatValue(ctx, ret, recurseTimes); | ||
| } | ||
| return ret.replace(/\n/g, `\n${" ".repeat(ctx.indentationLvl)}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It would be better to keep the customInspect support for compatibility.
There was a problem hiding this comment.
The idea here is to align Deno.inspect and util.inspect into the single implementation as they have lot of duplicated algorithms.
I think we should move these (Node.js specific) custom inspect features into Deno.inspect implementation in later PRs.
|
Note: Deno.inspect uses double quote and util.inspect uses single quote for string literals. This difference breaks some compat test cases |
| @@ -752,7 +752,7 @@ class EventTarget extends WebEventTarget { | |||
| return name; | |||
| } | |||
|
|
|||
| const opts = ObjectAssign({}, options, { | |||
| const opts = Object.assign({}, options, { | |||
There was a problem hiding this comment.
Why change this primordial?
There was a problem hiding this comment.
ObjectAssign wasn't available in this scope, but probably we should fix this in the opposite way. I'll update
There was a problem hiding this comment.
I'm not sure I understand - there should be no problem getting primordials from const primordials = globalThis.__bootstrap.primordials;
There was a problem hiding this comment.
@kt3k looks good in principal - is there a clear way forward to enable the tests you commented for now?
I think there's no obvious path to it ( |
This reverts commit a3529d0.
This reverts commit a3529d0. This change made debugging Node tests very hard - `AssertionError` is now printed as `[Circular *1]` giving no visibility what failed. We need to align two implementations together and remove this one then.
This reverts commit a3529d0. This change made debugging Node tests very hard - `AssertionError` is now printed as `[Circular *1]` giving no visibility what failed. We need to align two implementations together and remove this one then.
No need for two almost identical implementations of the same thing