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

Use util.inspect.custom symbol if available. #204

Merged
merged 4 commits into from
Aug 13, 2019

Conversation

goto-bus-stop
Copy link
Contributor

Ports the buffer-inspect test from Node core.
Adds a Buffer.prototype[util.inspect.custom] method, which is an alias
to Buffer.prototype.inspect. Node already doesn't have an .inspect
method on Buffers anymore, but since this module has to work in browsers
that do not have Symbols, it seems better to keep it around.

In Node, this will use the builtin util.inspect.custom symbol. In the
browser, it will use Symbol.for('util.inspect.custom'). The browser
version of util will also use the inspect-custom-symbol module in
the near future.

If nodejs/node#20857 gets merged,
Symbol.for('util.inspect.custom') will be used everywhere and the
dependency on inspect-custom-symbol could probably be dropped.

The motivation for this is API parity and the fact that Node is
removing support for the old .inspect method:
nodejs/node#20722

Ports the `buffer-inspect` test from Node core.
Adds a `Buffer.prototype[util.inspect.custom]` method, which is an alias
to `Buffer.prototype.inspect`. Node already doesn't have an `.inspect`
method on Buffers anymore, but since this module has to work in browsers
that do not have Symbols, it seems better to keep it around.

In Node, this will use the builtin `util.inspect.custom` symbol. In the
browser, it will use `Symbol.for('util.inspect.custom')`. The browser
version of `util` will also use the `inspect-custom-symbol` module in
the near future.

If nodejs/node#20857 gets merged,
`Symbol.for('util.inspect.custom')` will be used everywhere and the
dependency on `inspect-custom-symbol` could probably be dropped.

The motivation for this is API parity and the fact that Node is
removing support for the old `.inspect` method:
nodejs/node#20722
@feross feross mentioned this pull request Aug 12, 2019
@feross
Copy link
Owner

feross commented Aug 12, 2019

Hi @goto-bus-stop,

I'm down to merge this PR. But I noticed that Node.js ended up going with Symbol.for('nodejs.util.inspect.custom') and the https://github.com/mafintosh/inspect-custom-symbol package still uses Symbol.for('util.inspect.custom'). What do you think about just skipping the package using Symbol.for directly?

Thanks for pointing out that buffer.inspect has been deprecated. We can remove that in another PR. I opened an issue here: #239

@goto-bus-stop
Copy link
Contributor Author

Will try to update later today 👍

Copy link
Owner

@feross feross left a comment

Choose a reason for hiding this comment

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

LGTM

@feross feross merged commit 8528f1c into feross:master Aug 13, 2019
@feross
Copy link
Owner

feross commented Aug 13, 2019

5.4.0

@goto-bus-stop goto-bus-stop deleted the inspect-symbol branch August 13, 2019 19:57
@fanatid
Copy link
Contributor

fanatid commented Aug 13, 2019

Not sure how this relevant and correct, but Symbol.for was available from Chrome>=40, while Symbol was from Chrome>=38
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol#Browser_compatibility

Maybe?

typeof Symbol !== 'undefined' && typeof Symbol.for === 'function'

@feross
Copy link
Owner

feross commented Aug 13, 2019

@fanatid Nice catch. However, Chrome 40 was released on 2015-01-21. I don't think anyone is still running that version, and we only officially support the last 2 versions of Chrome.

@feross
Copy link
Owner

feross commented Aug 27, 2019

@fanatid It seems like the issue you identified affects some users of Cordova on old versions of Android. #241 I guess we can include this after all since it's just slightly longer and will ensure this works in more places :)

@ljharb
Copy link
Contributor

ljharb commented Dec 20, 2019

Symbol.for is a syntax error in older browsers; I filed #254 to use bracket notation to avoid that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants