-
-
Notifications
You must be signed in to change notification settings - Fork 6
Fix #159: support isPlainObj in IE11 and constructor.name props #194
Conversation
To my suprise, it's even faster in Chrome and FF: https://jsben.ch/ApnQr |
src/utils/isPlainObj.js
Outdated
value.constructor.name === 'Object') | ||
); | ||
export default function isPlainObject(value) { | ||
if (!value || typeof value !== '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.
Should we use toString
to fail more quickly for some objects (e.g., dates, array, etc.)?
if (!value || typeof value !== 'object') { | |
if (!value || typeof value !== 'object' || Object.prototype.toString.call(value) !== '[object 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.
Actually I think this change is necessary in order for these test cases (from the lodash
tests) to pass:
it('should return `false` for non-Object objects', () => {
expect(isPlainObject(arguments)).toBe(false);
expect(isPlainObject(Math)).toBe(false);
});
it('should return `false` for objects with a read-only `Symbol.toStringTag` property', () => {
const object = {};
Object.defineProperty(object, Symbol.toStringTag, {
'configurable': true,
'enumerable': false,
'writable': false,
'value': 'X'
});
expect(isPlainObject(object)).toBe(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.
The toString method is a weak workaround for IE8 and lower, because it does not support getPrototypeOf
.
But calling functions on unknown objects is a stupid way to deal with anything. It is likely slowing things down needlessly and depends on the output of toString, which is not only dependent on browser type and version, but can be customized by everyone! Existence/Absence and internas of such a function are simply not reliable.
'[object Object]'
is what non- ES5 browsers like IE8 used to return
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.
Just stop yourself right there. IE8 and lower! What year is it again? We're supposed to be living in a world with Replicants where LA is a megacity with corporations in pyramids!
The thing is, immutable doesn't even really work right on IE8 anyway. It's always been a collection of godawful hacks. And it's appreciable that they've done it, but anyone still stuck with IE8 can also be stuck with immutable@3
. Given that we can't have it both ways without making sacrifices, I think it's absolutely clear what we should sacrifice : )
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.
Furthermore using Object.getPrototypeOf
isn't even dropping support for IE8. IE8 had __proto__
and thus is capable of receiving a polyfill for that functionality. Anyone still supporting it had BETTER be including those polyfills through CoreJS, and if they aren't, well you'd think that the major version number (and having to change the package name) might tip them off to the potential for breaking changes. We just need to update the docs and also we really aught to have a CHANGELOG, though I don't relish the thought of trying to create one retroactively for changes that I didn't make.
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.
Besides a changelog we should somewhere mention what we think we support. I mean it's already suprising to have an NPM lib that works without babel :)
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.
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 would generally claim that it's your own fault if you expect Immutable.Map(Math)
or fromJS(Math)
to produce anything useful. I added the Object.prototype.toString
call as it is not affected by custom prototypes/objects having a custom toString function.
I could imagine someone overwriting Object.prototype.toString
to prettyprint stuff, but I guess they would be equally to blame as the guy writing fromJS(Math)
. At least fromJS(arguments) could be remotely useful.
src/utils/isPlainObj.js
Outdated
const proto = Object.getPrototypeOf(value); | ||
return proto === Object.prototype || proto === 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 noticed that lodash
is walking up the prototype chain when comparing the prototype. Do we have worry about that? Looks like it was added in this PR although I didn't dig into differing contexts enough to understand why it's necessary.
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.
Oh because if you have multiple frames you can essentially have Object.prototype !== Object.prototype
. That code ensures that an object can still be considered "plain" even if it has a different prototype than your realm's Object.prototype
. I'd say there's no reason not to incorporate that improvment.
Doesn't look like github picks up issue numbers in the PR title. I think they have to be in the commit message body with appropriate verbiage, e.g. I vote we just copy pasta the lodash implementation. |
I disagree, and you kinda do too in the comments above? Lodash has a different target. It checks the whole tree, which is not the task at hand. The second place it is used is in BTW the lodash implementation fails to detect Object.create(null).. as plain object. |
Last push just changed the message to link the issue using the correct verb |
Pretty sure the lodash implementation correctly detects |
Lodash doesn't have a different target, and it doesn't actually check the whole tree. It just walks to the bottom to find an |
8e5496b
to
cfeebcd
Compare
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.
Thanks! Might as well have @conartist6 approve it as well.
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.
Ship it.
Deals with IE9-IE11 and correctly evaluates
{ constructor: { name: 'bamboozled' }}
I am not that happy with exporting
isPlainObject
, if you see a better alternative to test it, be my guest