Printing the constructor name for non-plain objects #15
Conversation
DanielSWolf
force-pushed
the
feature/typed-objects
branch
from
be1c8d7
to
51c9014
Compare
| @@ -8,6 +8,7 @@ var setSize = hasSet && setSizeDescriptor && typeof setSizeDescriptor.get === 'f | |||
| var setForEach = hasSet && Set.prototype.forEach; | |||
| var booleanValueOf = Boolean.prototype.valueOf; | |||
| var objectToString = Object.prototype.toString; | |||
| var getPrototype = Object.getPrototypeOf || function(o) { return o.__proto__; }; | |||
There was a problem hiding this comment.
What about the older engines where __proto__ isn't even available?
There was a problem hiding this comment.
In that case, __proto__ will be undefined, as will be the result of getPrototype. Consequently, getTypeString will return null (because the result of getPrototype is falsey). And the code will behave as it always has, not printing a type prefix.
There was a problem hiding this comment.
Wouldn't a { __proto__: 3 } return 3 in an engine where it's not available?
There was a problem hiding this comment.
Wouldn't a
{ __proto__: 3 }return3in an engine where it's not available?
That will happen even in engines that support __proto__.
There was a problem hiding this comment.
@ExE-Boss that spec text only treats it as a normal data property when it's not a string, or computed.
({ __proto__: 3 }).__proto__ === Object.prototype returns true in a modern engine, but in, say, IE 8, it returns false.
There was a problem hiding this comment.
Specifically, in IE 8, ({ __proto__: 3 }).__proto__ returns 3.
index.js
Outdated
| // Returns the object's constructor name or null if it is a plain object | ||
| // or doesn't have a prototype. | ||
| function getTypeString(o) { | ||
| if (Object.prototype.toString(o) !== '[object Object]') return null; |
There was a problem hiding this comment.
I think you want .toString.call(o), otherwise this will always return true
There was a problem hiding this comment.
You're right, my bad. I'll fix this.
| && o.hasOwnProperty('hasOwnProperty') | ||
| && o.hasOwnProperty('isPrototypeOf') | ||
| && o.hasOwnProperty('propertyIsEnumerable') | ||
| && o.hasOwnProperty('toLocaleString') |
There was a problem hiding this comment.
should these check that they're functions, and ideally, that they behave as expected?
There was a problem hiding this comment.
I don't think there's anything to be gained by that. Consider that you have an object with own properties hasOwnProperty, isPrototypeOf, etc. Either this is Object.prototype, or it's an object that's trying really hard to look like it. In the latter case, chances are that all those will be functions and that they'll behave correctly.
What's the worst that can happen? The worst case is that you have an object that's trying hard to look like a plain object, and that your library prints it just like a plain object. I don't think that's bad.
After all, this library isn't generating structured information. It's generating a debug string that is supposed to help a developer understand an object graph. And I don't think that anybody will complain if something that's virtually indistinguishable from a plain object is visualized as a plain object.
There was a problem hiding this comment.
That's a fair point. However, to be indistinguishable, this library would have to be unable to distinguish it.
One extreme length we could go to is ensuring not just that they're all functions, but also that Function.prototype.toString.call indicates it's native code, which would work cross-realm.
ljharb
force-pushed
the
master
branch
3 times, most recently
from
95d2316
to
3ddb3e4
Compare
ljharb
force-pushed
the
feature/typed-objects
branch
from
1a19dca
to
12fdb40
Compare
Codecov Report
@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 98.14% 95.37% -2.77%
==========================================
Files 2 2
Lines 216 238 +22
Branches 82 98 +16
==========================================
+ Hits 212 227 +15
- Misses 4 11 +7
Continue to review full report at Codecov.
|
ljharb
force-pushed
the
master
branch
4 times, most recently
from
7363e8f
to
c2d7746
Compare
ljharb
force-pushed
the
main
branch
2 times, most recently
from
7ada44f
to
431bab2
Compare
Fixes #14