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

util: add util.types.isBoxedPrimitive #22620

Closed
wants to merge 1 commit into
base: master
from

Conversation

@BridgeAR
Member

BridgeAR commented Aug 31, 2018

Checking all boxed primitives individually requires to cross the C++
barrier multiple times besides being more complicated than just a
single check.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
util: add util.types.isBoxedPrimitive
Checking all boxed primitives individually requires to cross the C++
barrier multiple times besides being more complicated than just a
single check.

@BridgeAR BridgeAR requested a review from addaleax Aug 31, 2018

@BridgeAR BridgeAR referenced this pull request Aug 31, 2018

Closed

util: improve util inspect performance #22503

4 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Show comment
Hide comment
Member

BridgeAR commented Aug 31, 2018

@refack

refack approved these changes Aug 31, 2018

@@ -68,6 +69,15 @@ for (const [ value, _method ] of [
}
}
// Check boxed primitives.

This comment has been minimized.

@benjamingr

benjamingr Aug 31, 2018

Member

Maybe add some negative results to the test?

@benjamingr

benjamingr Aug 31, 2018

Member

Maybe add some negative results to the test?

This comment has been minimized.

@BridgeAR

BridgeAR Sep 3, 2018

Member

This seems somewhat redundant to me, since this is just a helper combined out of different helpers that are already tested against all other types.
But I'll add negative results as well if you feel strongly about it!

@BridgeAR

BridgeAR Sep 3, 2018

Member

This seems somewhat redundant to me, since this is just a helper combined out of different helpers that are already tested against all other types.
But I'll add negative results as well if you feel strongly about it!

This comment has been minimized.

@benjamingr

benjamingr Sep 4, 2018

Member

I think it makes sense to test - but don't feel strongly about it.

@benjamingr

benjamingr Sep 4, 2018

Member

I think it makes sense to test - but don't feel strongly about it.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 1, 2018

Member

Maybe name it isPrimitiveObject? I don't see the term "boxed" being used in the ecmascript specification or on MDN.

Edit: It's more consistent with the names of the individual methods (there is no isBoxedBoolean)

Member

targos commented Sep 1, 2018

Maybe name it isPrimitiveObject? I don't see the term "boxed" being used in the ecmascript specification or on MDN.

Edit: It's more consistent with the names of the individual methods (there is no isBoxedBoolean)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 1, 2018

Member

@targos You'll sometimes see boxed, unboxed, and auto-boxing come up in conversations. It's used to describe what happens when you access a property on a primitive value. For example:

"a".trim // auto-boxes primitive "a" to access the property "trim"
"a".a = 1 // box primitive for property assignment then unbox after
"a".a // undefined because the property was assigned to the boxed value which was discarded

MDN mentions boxing here , here, and here.

That said, isPrimitiveObject is more precise so 👍

Member

jdalton commented Sep 1, 2018

@targos You'll sometimes see boxed, unboxed, and auto-boxing come up in conversations. It's used to describe what happens when you access a property on a primitive value. For example:

"a".trim // auto-boxes primitive "a" to access the property "trim"
"a".a = 1 // box primitive for property assignment then unbox after
"a".a // undefined because the property was assigned to the boxed value which was discarded

MDN mentions boxing here , here, and here.

That said, isPrimitiveObject is more precise so 👍

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 3, 2018

Member

isPrimitiveObject sounds somewhat wrong to me. Turning it around to isObjectPrimitive seems somewhat better but still not awesome (isPrimitiveObject reminds me of an object that is primitive and I would not know what that should be. It somewhat reminds me of an plain object). However, I can not think of anything else as alternative and it does align with the other is...Object functions. Anyone else with more suggestions / reasons for either name?

Member

BridgeAR commented Sep 3, 2018

isPrimitiveObject sounds somewhat wrong to me. Turning it around to isObjectPrimitive seems somewhat better but still not awesome (isPrimitiveObject reminds me of an object that is primitive and I would not know what that should be. It somewhat reminds me of an plain object). However, I can not think of anything else as alternative and it does align with the other is...Object functions. Anyone else with more suggestions / reasons for either name?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 3, 2018

Member

Turning it around to isObjectPrimitive seems somewhat better but still not awesome

That would imply that we’re checking for “object primitives”, i.e. primitives which have something to do with objects. The values that pass the tests are definitely not primitives, though.

Member

addaleax commented Sep 3, 2018

Turning it around to isObjectPrimitive seems somewhat better but still not awesome

That would imply that we’re checking for “object primitives”, i.e. primitives which have something to do with objects. The values that pass the tests are definitely not primitives, though.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 3, 2018

Member

@addaleax

primitives which have something to do with objects

It does seems somewhat more intuitive for me (even if it's not really a primitive it is why we check for it: we want to know if it has something to do with a primitive by encapsulating one). I never heard of an "primitive object" and my head just can't get used to that name.

Member

BridgeAR commented Sep 3, 2018

@addaleax

primitives which have something to do with objects

It does seems somewhat more intuitive for me (even if it's not really a primitive it is why we check for it: we want to know if it has something to do with a primitive by encapsulating one). I never heard of an "primitive object" and my head just can't get used to that name.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 3, 2018

Member

I never heard of an "primitive object" and my head just can't get used to that name.

I agree that it sounds odd, but I think it’s more accurate than “object primitive”. Tbh, I think isBoxedObject or isBoxedPrimitive both are okay, even if the spec doesn’t talk about boxing with that term.

Member

addaleax commented Sep 3, 2018

I never heard of an "primitive object" and my head just can't get used to that name.

I agree that it sounds odd, but I think it’s more accurate than “object primitive”. Tbh, I think isBoxedObject or isBoxedPrimitive both are okay, even if the spec doesn’t talk about boxing with that term.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Sep 3, 2018

Member

(it was just a suggestion, I'm not blocking anything. Just wanted to make sure alternative names were considered)

Member

targos commented Sep 3, 2018

(it was just a suggestion, I'm not blocking anything. Just wanted to make sure alternative names were considered)

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 3, 2018

Member

I am fine with either isBoxedPrimitive or isBoxedObject while having a preference for the former.

Member

BridgeAR commented Sep 3, 2018

I am fine with either isBoxedPrimitive or isBoxedObject while having a preference for the former.

@devsnek

devsnek approved these changes Sep 3, 2018

isBoxedPrimitive sounds good to me. :shipit:

I would also be okay with isObjectWithPrototypeThatIsOneOfThePrimitivePrototypeIntrinsics 😄

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 3, 2018

Member

I dig isPrimitiveObject because, as mentioned before, it fits with the existing naming convention of isXyzObject, like isBooleanObject and isNumberObject, by replacing Boolean or Number with Primitive. While more letters, Primitive for me is easier to grok at a glance than Boxed too.

Update:

I should make it clear I'm OK with the existing isBoxedPrimitive too
(isPrimitiveObject is a preference for me is all).

Member

jdalton commented Sep 3, 2018

I dig isPrimitiveObject because, as mentioned before, it fits with the existing naming convention of isXyzObject, like isBooleanObject and isNumberObject, by replacing Boolean or Number with Primitive. While more letters, Primitive for me is easier to grok at a glance than Boxed too.

Update:

I should make it clear I'm OK with the existing isBoxedPrimitive too
(isPrimitiveObject is a preference for me is all).

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Sep 3, 2018

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 3, 2018

It's a boxed primitive imo; a "primitive object" doesn't make sense because the two words are mutually exclusive. It's also not a boxed object, it's an object formed by boxing a primitive into an object.

(fwiw this is already polyfillable in pure JS; see https://twitter.com/ljharb/status/1036707165002559488)

ljharb commented Sep 3, 2018

It's a boxed primitive imo; a "primitive object" doesn't make sense because the two words are mutually exclusive. It's also not a boxed object, it's an object formed by boxing a primitive into an object.

(fwiw this is already polyfillable in pure JS; see https://twitter.com/ljharb/status/1036707165002559488)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 3, 2018

Member

@ljharb

(fwiw this is already polyfillable in pure JS; see

Yep, and we have util.types.isNumberObject and friends too. The addition in this PR, is part of exposing helpers used to make inspect perf better (it's handy for inspect and handy for those in the ecosystem too). See #22503.

Member

jdalton commented Sep 3, 2018

@ljharb

(fwiw this is already polyfillable in pure JS; see

Yep, and we have util.types.isNumberObject and friends too. The addition in this PR, is part of exposing helpers used to make inspect perf better (it's handy for inspect and handy for those in the ecosystem too). See #22503.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 3, 2018

@jdalton to clarify; that it's already polyfillable is why this is safe to expose directly as a faster and much more friendly helper :-) i'm super on board.

ljharb commented Sep 3, 2018

@jdalton to clarify; that it's already polyfillable is why this is safe to expose directly as a faster and much more friendly helper :-) i'm super on board.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR
Member

BridgeAR commented Sep 3, 2018

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Sep 3, 2018

Member

Would it be worthwhile to instead implement this in JavaScript? It seems like it's possible to do so, and it's usually faster than an implementation that goes through C++.

Member

TimothyGu commented Sep 3, 2018

Would it be worthwhile to instead implement this in JavaScript? It seems like it's possible to do so, and it's usually faster than an implementation that goes through C++.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 3, 2018

@TimothyGu that would require 4 try/catches around calling cached builtin methods, expecting them to throw for a true result. I would be very surprised if it was faster to do it in pure JS.

ljharb commented Sep 3, 2018

@TimothyGu that would require 4 try/catches around calling cached builtin methods, expecting them to throw for a true result. I would be very surprised if it was faster to do it in pure JS.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 3, 2018

Member

I just ran a simple benchmark only for boxed numbers and it's ten times faster in C++ (this is mixed input. Using only boxed numbers as input it's two times slower):

tryNumberObject: 1255.014ms
isNumberObject: 130.553ms

Note: I optimized tryNumberObject before using it by setting the stackTraceLimit to 0. Otherwise it would be 20 times slower.

Member

BridgeAR commented Sep 3, 2018

I just ran a simple benchmark only for boxed numbers and it's ten times faster in C++ (this is mixed input. Using only boxed numbers as input it's two times slower):

tryNumberObject: 1255.014ms
isNumberObject: 130.553ms

Note: I optimized tryNumberObject before using it by setting the stackTraceLimit to 0. Otherwise it would be 20 times slower.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 3, 2018

Member

@TimothyGu that would require 4 try/catches around calling cached builtin methods, expecting them to throw for a true result. I would be very surprised if it was faster to do it in pure JS.

Would you mind explaining why @ljharb?

Member

benjamingr commented Sep 3, 2018

@TimothyGu that would require 4 try/catches around calling cached builtin methods, expecting them to throw for a true result. I would be very surprised if it was faster to do it in pure JS.

Would you mind explaining why @ljharb?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 3, 2018

@benjamingr because the only way to determine if something is a boxed primitive in JS, cross-realm, is to .call a brand-checking prototype method on it, and if it throws, it is one. (instanceof is never reliable) So, the algorithm would have to be basically this:

  1. return false if it's a primitive, or a function
  2. try/catch to see if it's got a Boolean slot, if so, return true
  3. try/catch to see if it's got a String slot, if so, return true
  4. try/catch to see if it's got a Symbol slot, if so, return true
  5. try/catch to see if it's got a Number slot, if so, return true
  6. return false

ljharb commented Sep 3, 2018

@benjamingr because the only way to determine if something is a boxed primitive in JS, cross-realm, is to .call a brand-checking prototype method on it, and if it throws, it is one. (instanceof is never reliable) So, the algorithm would have to be basically this:

  1. return false if it's a primitive, or a function
  2. try/catch to see if it's got a Boolean slot, if so, return true
  3. try/catch to see if it's got a String slot, if so, return true
  4. try/catch to see if it's got a Symbol slot, if so, return true
  5. try/catch to see if it's got a Number slot, if so, return true
  6. return false
@trivikr

trivikr approved these changes Sep 4, 2018

@jasnell

jasnell approved these changes Sep 4, 2018

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 4, 2018

Member

If no one objects to the current name in the next 24h, I'll go ahead and land it.

The twitter poll resulted in 26% for the current name, 26% for isObjectPrimitive and 40% for isPrimitiveObject and 8% for isBoxedObject. Since the poll can not show how strongly people prefer either one, it's hopefully fine to just keep the current name.

Member

BridgeAR commented Sep 4, 2018

If no one objects to the current name in the next 24h, I'll go ahead and land it.

The twitter poll resulted in 26% for the current name, 26% for isObjectPrimitive and 40% for isPrimitiveObject and 8% for isBoxedObject. Since the poll can not show how strongly people prefer either one, it's hopefully fine to just keep the current name.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 5, 2018

util: add util.types.isBoxedPrimitive
Checking all boxed primitives individually requires to cross the C++
barrier multiple times besides being more complicated than just a
single check.

PR-URL: nodejs#22620
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 5, 2018

Member

Thanks everyone.

Landed in 3209679 🎉

Member

BridgeAR commented Sep 5, 2018

Thanks everyone.

Landed in 3209679 🎉

@BridgeAR BridgeAR closed this Sep 5, 2018

targos added a commit that referenced this pull request Sep 6, 2018

util: add util.types.isBoxedPrimitive
Checking all boxed primitives individually requires to cross the C++
barrier multiple times besides being more complicated than just a
single check.

PR-URL: #22620
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Sep 18, 2018

2018-09-19, Version 10.11.0 (Current)
Notable changes:

* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to `true`, non-existing parent folders will be
    automatically created.
    #21875
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom `require` function that will resolve
    modules relative to the `filename` path.
    #19360
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between `file:` URLs and
    absolute paths.
    #22506
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

targos added a commit that referenced this pull request Sep 18, 2018

2018-09-19, Version 10.11.0 (Current)
Notable changes:

* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to `true`, non-existing parent folders will be
    automatically created.
    #21875
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom `require` function that will resolve
    modules relative to the `filename` path.
    #19360
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between `file:` URLs and
    absolute paths.
    #22506
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

PR-URL: #22932

targos added a commit that referenced this pull request Sep 19, 2018

2018-09-19, Version 10.11.0 (Current)
Notable changes:

* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to `true`, non-existing parent folders will be
    automatically created.
    #21875
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom `require` function that will resolve
    modules relative to the `filename` path.
    #19360
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between `file:` URLs and
    absolute paths.
    #22506
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

PR-URL: #22932

targos added a commit that referenced this pull request Sep 19, 2018

2018-09-19, Version 10.11.0 (Current)
Notable changes:

* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to `true`, non-existing parent folders will be
    automatically created.
    #21875
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom `require` function that will resolve
    modules relative to the `filename` path.
    #19360
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between `file:` URLs and
    absolute paths.
    #22506
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

PR-URL: #22932

targos added a commit that referenced this pull request Sep 20, 2018

2018-09-20, Version 10.11.0 (Current)
Notable changes:

* fs
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

PR-URL: #22932

targos added a commit that referenced this pull request Sep 20, 2018

2018-09-20, Version 10.11.0 (Current)
Notable changes:

* fs
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

PR-URL: #22932

targos added a commit that referenced this pull request Sep 20, 2018

2018-09-20, Version 10.11.0 (Current)
Notable changes:

* fs
  * Fixed fsPromises.readdir `withFileTypes`.
    #22832
* http2
  * Added `http2stream.endAfterHeaders` property.
    #22843
* util
  * Added `util.types.isBoxedPrimitive(value)`.
    #22620
* Added new collaborators:
  * boneskull (https://github.com/boneskull) - Christopher Hiller
* The Technical Steering Committee has new members:
  * apapirovski (https://github.com/apapirovski) - Anatoli Papirovski
  * gabrielschulhof (https://github.com/gabrielschulhof) - Gabriel Schulhof

PR-URL: #22932
@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 20, 2018

Member

Is this only useful for isolated performance hacks? I'm trying to think of a use-case for it and can't, beyond the kinds of things that go in to util.inspect() as mentioned. Seems like an intentional leak on an abstraction that isn't intended to be leaky. Not a criticism, I'm just puzzled.

Member

rvagg commented Sep 20, 2018

Is this only useful for isolated performance hacks? I'm trying to think of a use-case for it and can't, beyond the kinds of things that go in to util.inspect() as mentioned. Seems like an intentional leak on an abstraction that isn't intended to be leaky. Not a criticism, I'm just puzzled.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Sep 20, 2018

Member

Seems like an intentional leak on an abstraction that isn't intended to be leaky.

> var x= new Boolean()
> x
[Boolean: false]
> var y = false
> y
false
> typeof x
'object'
> typeof y
'boolean'
> x === y
false

I believe it's just an aggregate of checks already available in the language. But yeah, util and util.inspect provide abstraction breaking footguns, but they are explicit about it.

The util.inspect() method returns a string representation of object that is intended for debugging. The output of util.inspect may change at any time and should not be depended upon programmatically.

Please note that util.inspect() is a synchronous method that is mainly intended as a debugging tool. Some input values can have a significant performance overhead that can block the event loop. Use this function with care and never in a hot code path.

Member

refack commented Sep 20, 2018

Seems like an intentional leak on an abstraction that isn't intended to be leaky.

> var x= new Boolean()
> x
[Boolean: false]
> var y = false
> y
false
> typeof x
'object'
> typeof y
'boolean'
> x === y
false

I believe it's just an aggregate of checks already available in the language. But yeah, util and util.inspect provide abstraction breaking footguns, but they are explicit about it.

The util.inspect() method returns a string representation of object that is intended for debugging. The output of util.inspect may change at any time and should not be depended upon programmatically.

Please note that util.inspect() is a synchronous method that is mainly intended as a debugging tool. Some input values can have a significant performance overhead that can block the event loop. Use this function with care and never in a hot code path.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Sep 20, 2018

Member

As a utility lib author this method is exciting because I so run into a need for this kind of check (equality checks, cloning, merging, etc) and would rather defer to an environment builtin when possible.

Member

jdalton commented Sep 20, 2018

As a utility lib author this method is exciting because I so run into a need for this kind of check (equality checks, cloning, merging, etc) and would rather defer to an environment builtin when possible.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 20, 2018

Member

I did not implement it for performance reasons in the first place. I mainly want to have a utility function that makes sure I know if a variable is a boxed primitive or not. No matter what primitive it is. Otherwise I have a bloated code where I have to go through each boxed primitive manually / write an abstraction for it.

Doing it in core seemed the best way of doing it because we do less boundary crossings and it fits well into all the other util.types functions. We have something similar with IsAnyArrayBuffer.

Member

BridgeAR commented Sep 20, 2018

I did not implement it for performance reasons in the first place. I mainly want to have a utility function that makes sure I know if a variable is a boxed primitive or not. No matter what primitive it is. Otherwise I have a bloated code where I have to go through each boxed primitive manually / write an abstraction for it.

Doing it in core seemed the best way of doing it because we do less boundary crossings and it fits well into all the other util.types functions. We have something similar with IsAnyArrayBuffer.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 20, 2018

Member

OK, I get it, thanks for the insight folks!

Member

rvagg commented Sep 20, 2018

OK, I get it, thanks for the insight folks!

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 21, 2018

For those interested, I ended up making a package for this: https://www.npmjs.com/package/is-boxed-primitive

ljharb commented Sep 21, 2018

For those interested, I ended up making a package for this: https://www.npmjs.com/package/is-boxed-primitive

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

util: add util.types.isBoxedPrimitive
Checking all boxed primitives individually requires to cross the C++
barrier multiple times besides being more complicated than just a
single check.

PR-URL: nodejs#22620
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment