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

Pass util.inspect to custom inspect methods #35956

Closed
Jamesernator opened this issue Nov 4, 2020 · 1 comment · Fixed by #41019
Closed

Pass util.inspect to custom inspect methods #35956

Jamesernator opened this issue Nov 4, 2020 · 1 comment · Fixed by #41019
Labels
util Issues and PRs related to the built-in util module.

Comments

@Jamesernator
Copy link

Currently when using [Symbol.for('nodejs.util.inspect.custom')](depth, opts) in cross-environment code we have a problem when wanting to use the results of util.inspect on inner values of our value.

For example consider an implementation of Complex like so:

class Complex {
  #real;
  #imag;

  constructor(real, imag) {
    this.#real = real;
    this.#imag = imag;
  }

  get real() {
    return this.#real;
  }

  get imag() {
    return this.#imag;
  }
}

If we wish to extend it with [Symbol.for('nodejs.util.custom.inspect')] we can do something like the following:

class Complex {
  // ...
  [Symbol.for('nodejs.util.inspect.custom')]() {
    return `Complex { real: ${ this.#real }, imag: ${ this.#imag } }`
  }
}

However this has the downside of losing the color-formatting on the numbers. In order to fix this we could import util.inspect and use it as follows:

import { inspect } from 'util';

class Complex {
  // ...
  [Symbol.for('nodejs.util.inspect.custom')]() {
    return `Complex { real: ${ inspect(this.#real) }, imag: ${ inspect(this.#imag) } }`
  }
}

However now we have made our code non-portable by importing util, even though this code has no other dependencies on Node.

As such I'd like to propose passing inspect into [Symbol.for('nodejs.util.inspect.custom')]() as the third argument so that we could write it like so:

class Complex {
  // ...
  [Symbol.for('nodejs.util.inspect.custom')](depth, opts, inspect) {
    return `Complex { real: ${ inspect(this.#real) }, imag: ${ inspect(this.#imag) } }`;
  }
@aduh95
Copy link
Contributor

aduh95 commented Nov 4, 2020

Do you want to submit a PR implementing this change?

@PoojaDurgad PoojaDurgad added the util Issues and PRs related to the built-in util module. label Dec 18, 2020
BridgeAR added a commit to BridgeAR/node that referenced this issue Nov 29, 2021
This allows to use more portable custom inspect functions.

Fixes: nodejs#35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit to BridgeAR/node that referenced this issue Dec 4, 2021
This allows to use more portable custom inspect functions.

Fixes: nodejs#35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
nodejs-github-bot pushed a commit that referenced this issue Dec 11, 2021
This allows to use more portable custom inspect functions.

Fixes: #35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41019
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Dec 14, 2021
This allows to use more portable custom inspect functions.

Fixes: #35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41019
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
This allows to use more portable custom inspect functions.

Fixes: #35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41019
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
This allows to use more portable custom inspect functions.

Fixes: #35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41019
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
This allows to use more portable custom inspect functions.

Fixes: nodejs#35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#41019
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
This allows to use more portable custom inspect functions.

Fixes: #35956

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #41019
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Offroaders123 added a commit to Offroaders123/NBTify that referenced this issue May 9, 2023
This is the custom logging behavior mentioned in the previous commit, it's super neat! I've wanted to learn how to do this for a while now, and I ended up finding the right documentation which described everything I needed to do in order to get it to work.

It's a Node-specific thing, so this is meant for that.

Essentially, you can customize all of the ways that Node will log a given object, and you can specify things like the actual text that comes out in the log, what color it should be, and things like that. Using that, I essentially made it so the content of the `NBTData.prototype.data` property is shown on the `NBTData` log itself, rather than all of the properties present on `NBTData`. The only thing I don't like about this is that it could make NBTify users think that the `NBTData` object itself directly holds the keys/values, when it's actually the `data` property that has them. I think this does look nicer in terms of the console output though.

Here's some demo code to set it up:

```ts
// https://stackoverflow.com/questions/10729276/how-can-i-get-the-full-object-in-node-jss-console-log-rather-than-object
// https://nodejs.org/api/util.html#utilinspectcustom
// nodejs/node#35956

import type { inspect as inspectFn, InspectOptions } from "node:util";

export class Byte extends Number {
  constructor(value: number) {
    super(value);
  }

  get [Symbol.toStringTag]() {
    return "Byte" as const;
  }
}

export class Short extends Number {
  constructor(value: number) {
    super(value);
  }

  get [Symbol.toStringTag]() {
    return "Short" as const;
  }

  // [inspect.custom]() {
  [Symbol.for("nodejs.util.inspect.custom")](depth: number, options: InspectOptions, inspect: typeof inspectFn) {
    return `${this[Symbol.toStringTag]} { ${inspect(this.valueOf(),{ colors: true })} }`;
  }
}

console.log(Number(16));
console.log(new Number(42));
console.log(new Byte(25));
console.log(new Short(83));
console.log(new Uint8Array(12));
```

After discovering this, I happened to look into the BigInt primitive wrapper extending class problem (it's not actually a constructor, so you can't extend it using the `class` syntax, 'kind of', I've come to learn and find out :) ).

Not sure which way I will take this, as to whether I will move the `LongTag` type over to it's own primitive wrapper class (that's been keeping the API in limbo, in-between either all primitive wrapper classes, or only primitives themselves). I think it's been a nice sweet spot to rely on types only to do things, and to use library module-specific functions to deal with the primitives, instead of doing everything with wrapper objects and methods. Now that I have this demo working though, I think I want to try out the primitive wrapper object syntax again. Not sure how much I like it though. It does at least kind of 'seem' like it could make the library simpler, but I'm not so sure about that, having learned more of the downfalls with OO programming.

```ts
// tc39/proposal-bigint#98
// https://stackoverflow.com/questions/3350215/what-is-returned-from-a-constructor
// microsoft/TypeScript#7285

import type { InspectOptionsStylized } from "node:util";

declare global {
  interface BigIntConstructor {
    new (value: bigint | boolean | number | string): BigInt;
  }
}

export class Long<T extends bigint = bigint> extends BigInt {
  // @ts-expect-error
  constructor(value: T) {
    return Object.setPrototypeOf(Object(BigInt(value)),Long.prototype);
  }

  override valueOf() {
    return super.valueOf();
  }

  override toString() {
    return `${this.valueOf()}l`;
  }

  // @ts-expect-error
  get [Symbol.toStringTag]() {
    return "Long" as const;
  }

  [Symbol.for("nodejs.util.inspect.custom")](_: number, { stylize }: InspectOptionsStylized) {
    return stylize(this.toString(),"bigint");
  }
}

const myThing = new Long(25n);
console.log(myThing,25n);
console.log(myThing instanceof BigInt);
```

Yeah, so I'm trying to decide whether to move the logic for doing things into wrapper classes, or to try and keep raw primitives wherever possible. This could essentially strengthen NBTify's associations between the tag types, and the classes/primitives themselves, which I think is what I like out of this.

For example, the `getTagType()` function currently uses a mix of `typeof` and `instanceof` checks to see if that value is one of the NBTify primitives/wrapper objects. If everything is made with wrapper objects, you could instead just use `instanceof` directly, and check if the value is an instanceof the given 'NBTify primitive', essentially any of the wrapper objects for all of the tag types, in the case that everything would be primitive wrapper objects. This could take some of the logic out of the type checking, and into the data structures themselves, or something like that.

I think one of the other things that I liked about the idea for primitive wrapper objects too, is that you could add your own `toString()` methods, and they could narrow down to plain SNBT, which NBTify could them parse directly. You might not need to have a `stringify()` function; at least, you could even just wrap the `toString()` of the root `CompoundTag` to be returned from `stringify()`, and the recursive nature of the data tree's `toString()` methods would manage the stringification of the tree, rather than the `stringify()` function itself. This moves the stringification logic out of the function, and onto the objects to handle themselves.

But for the already-existing wrapper objects, like the TypedArrays, I kind of don't like having to call a custom NBTify constructor just to create that NBT primitive type. It's only necessary to do for the single-number types, because they don't have JavaScript equivalents or alternatives.

That last paragraph made me think some more, what would JavaScript do? What's the best scenario here?

If this were JavaScript's realm, it would probably have the primitive types for these built-in. And if they are primitive types, they wouldn't be used by default as wrapper objects, you'd use them as the primitives themselves, just like how `string`, `number`, `bigint`, `boolean`, and `symbol` work right now. They may have wrapper objects, but you don't actually use them in your regular old code like that. You just use their primitives. If that's the case, and JavaScript *did* have those primitives built-in, then I wouldn't be using primitive wrapper objects for everything. Seeing it that way, it's obvious.

The best scenario would be for JavaScript to support the lower-level number primitives, `int8`, `int16`, `int32`, and `float32`. That would allow me to remove the number wrapper types, because the language and runtime itself could distinguish the values of each as different from each other. That's been my main reason for choosing to make wrapper objects for those. I didn't want to lose any type information from the NBT tree when parsing them as numbers in JavaScript. The `number` type itself can handle all of those values, they just can't be distinguished from one another. NBTify could always just assume what it should be, based on it's size, but this isn't safe, because this would essentially just coalesce everything to the smallest number bracket that it could fit into (most of the time `ByteTag` in NBT). And trying to guess the type is harder than already having it provided to you, by the source data itself. And if the primitives were available in JavaScript, that would be the simplest option. The only reason this one is in similar complexity to guessing the data type container, is because I have to re-create the data containers in JavaScript, because the primitives aren't (yet?) available.

So, with that whole thing in mind, I think I should likely stay away from the full primitive wrapper object route, because while it does seem to break up the logic into nice components, it inherently prevents you from using the data as primitives themselves, and it requires you to use objects to wrap the data itself, while it does break it up into nice containing blocks.

Ok, that was a lot of writing. Wasn't sure if I covered everything, but I'm happy how much I did. Listening to parts of Marco's new 'Their Colors Fade' album today, not sure how many times I've listened through all the different parts just today. I really like it a lot. I'm hearing more parts of his earlier albums that I hadn't noticed before, and it feels like a nice reference to the past. It definitely feels like a whole new concept and vibe, I really like it. The songs that are newer to me are really starting to catch on now. I knew a good few of them from during the time he was posting about working on the album online, so I think that's why the album has caught on so quick for me. It was familiar in understanding the vibe, but now it says something more specific as a whole when hearing them all together as one piece. He was right about the track list order, it did turn out perfect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants