-
-
Couldn't load subscription status.
- Fork 41
Printing the constructor name for non-plain objects #15
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
base: main
Are you sure you want to change the base?
Conversation
be1c8d7 to
51c9014
Compare
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the older engines where __proto__ isn't even available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a { __proto__: 3 } return 3 in an engine where it's not available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want .toString.call(o), otherwise this will always return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these check that they're functions, and ideally, that they behave as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
95d2316 to
3ddb3e4
Compare
1a19dca to
12fdb40
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #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 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7363e8f to
c2d7746
Compare
7ada44f to
431bab2
Compare
Fixes #14