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

Support util.inspect.custom as a public symbol #20821

Closed
chocolateboy opened this issue May 18, 2018 · 8 comments
Closed

Support util.inspect.custom as a public symbol #20821

chocolateboy opened this issue May 18, 2018 · 8 comments
Labels
feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.

Comments

@chocolateboy
Copy link
Contributor

  • Version: v10.1.0
  • Platform: Linux (Arch)
  • Subsystem: util

Please consider supporting util.inspect.custom as a public/global symbol either by:

  1. changing util.inspect.custom from a private symbol (e.g. Symbol('util.inspect.custom')) to a public symbol (e.g. Symbol.for('util.inspect.custom'))

or:

  1. accepting a public symbol as an alternative to the private symbol, just as "inspect" is currently accepted as a (deprecated) fallback for the symbol

Changing it from a string to a symbol was a great idea 🎉 and it's a perfect use case for a symbol, but, as mentioned here, making it private makes it painful to write code that both a) provides an inspect hook if it's loaded in node and b) works seamlessly in the browser if it's not.

A workaround has been implemented as an NPM module, inspect-custom-symbol (cc @mafintosh), which uses the browser field in its package.json to provide a substitute symbol without pulling in the util library in bundlers such as Browserify and Webpack. It's much better than trying to work around this with a tower of late-bound typeof util.inspect.custom checks, but it doesn't fix the underlying issue and it's not ideal:

  • it doesn't work in all situations/environments
  • it requires an extra dependency
  • most people with this dilemma won't be aware of it
@addaleax addaleax added util Issues and PRs related to the built-in util module. feature request Issues that request new features to be added to Node.js. labels May 18, 2018
@addaleax
Copy link
Member

I think the first option would be okay. Do you want to open a PR with it?

@chocolateboy
Copy link
Contributor Author

@addaleax Sure!

@targos
Copy link
Member

targos commented May 18, 2018

I like the idea of the first option!

@Yomguithereal
Copy link

I'm also in favor of the first option, though I am not sure to see if this would harm node's core as a side effect.

@TimothyGu
Copy link
Member

First option sounds good to me as well.

@Trott
Copy link
Member

Trott commented May 20, 2018

/ping @jakearchibald

@jakearchibald
Copy link

Using a global name kinda gives us the same problem we had the in the first place, it's just that the name is longer so less likely to clash with existing code.

However, it's probably the best thing to do for multi-realm environments like the browser.

@bradennapier
Copy link

bradennapier commented Jun 5, 2018

Yeah running into this now. Unfortunately all my proxies just print like this:

Set:  0 {}
Changes:  0
true
Set:  1 {}
Changes:  1
true
Set:  2 {}
Changes:  2
true
Set:  3 {}
Changes:  3
true

Since this lib should work everywhere, I dont want to import util, but would definitely like it to print the appropriate value! Glad to see progress is being made here. Only solution I could come up with makes me sad :( lol

// in the proxy get 
if (typeof key === 'symbol' && String(key) === 'Symbol(util.inspect.custom)') {
    // super hacky method of supporting nodejs printing values
    return () => descriptor.copy || descriptor.base;
  }

( Happy to hear of another solution if there is one :) )

Thanks for your hard work everyone!

@targos targos closed this as completed in dadd6e1 Sep 15, 2018
targos pushed a commit to targos/node that referenced this issue Sep 23, 2018
Define `util.inspect.custom` as
`Symbol.for("nodejs.util.inspect.custom")` rather than
`Symbol("util.inspect.custom")`. This allows `inspect` hooks to
easily/safely be defined in non-Node.js environments.

Fixes: nodejs#20821
Refs: nodejs#22684

PR-URL: nodejs#20857
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
targos pushed a commit that referenced this issue Sep 24, 2018
Define `util.inspect.custom` as
`Symbol.for("nodejs.util.inspect.custom")` rather than
`Symbol("util.inspect.custom")`. This allows `inspect` hooks to
easily/safely be defined in non-Node.js environments.

Fixes: #20821
Refs: #22684

Backport-PR-URL: #23039
PR-URL: #20857
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

8 participants