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

fix isPlainObject #306

Merged
merged 3 commits into from
Jul 23, 2015
Merged

fix isPlainObject #306

merged 3 commits into from
Jul 23, 2015

Conversation

michalkvasnicak
Copy link
Contributor

When isPlainObject is called between two different contexts then Object.prototype is not the same. Now it is comparing only stringified constructors. Fixes #304

…s instead of prototypes (did not work from iframes because Object in iframe is not the same as in parent)
@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2015

Can you add a test that would have been failing with the previous version?

@michalkvasnicak
Copy link
Contributor Author

I will try but don't know how to reproduce it inside the same context.

@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2015

Look into lodash tests? :-P

@phated
Copy link

phated commented Jul 23, 2015

Cc @jdalton

@michalkvasnicak
Copy link
Contributor Author

It seems that lodash is creating its own context.

@michalkvasnicak
Copy link
Contributor Author

OK I can reproduce it but had to install contextify for it. Can I push it with contextify as devDependency?

@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2015

Yes, sure.

…e plain object from another realm is checked
return false;
}

return typeof obj === 'object' &&
Object.getPrototypeOf(obj) === Object.prototype;
const proto = typeof obj.constructor === 'function' ? Object.getPrototypeOf(obj) : Object.prototype;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change const to var, we're maintaining Flow-friendliness for now. (I'll alter ESLint rule later.)

@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2015

Looks good, thank you!

@michalkvasnicak
Copy link
Contributor Author

Thank you for this awesome lib. :)

@jdalton
Copy link
Contributor

jdalton commented Jul 23, 2015

FWIW lodash v4 drops IE8 support and has revised its isPlainObject to be considerably more light weight. You caught us at a time in v3 when we pulled our modern method fork of isPlainObject for a single larger implementation.

It looks like this PR borrows from our WIP v4 implementation so you'll dig this method is designed to work with the es5-sham fallback for Object.getPrototypeOf too by punting if constructor is not a function.

@michalkvasnicak
Copy link
Contributor Author

@jdalton Yes it is, I was pointed to look at the lodash implementation. I hope it's OK with you.

@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2015

@jdalton Appreciate you chiming in!

gaearon added a commit that referenced this pull request Jul 23, 2015
@gaearon gaearon merged commit 0fc5802 into reduxjs:breaking-changes-1.0 Jul 23, 2015
@jdalton
Copy link
Contributor

jdalton commented Jul 23, 2015

@michalkvasnicak

I hope it's OK with you.

In the long term copy pasting is a bummer as it shifts the implementation and testing burden to you all. It also means you all have to spend time mucking with and becoming experts in utils instead of making cool stuff on top of them. It's on me to make the existing lodash utils more consumer friendly and file size is part of that. I hope this can be revisited in v4 where file size is a major focal point.

@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2015

@jdalton

I agree totally. In our case we wanted to get some micro file size optimizations because the whole lib is 2KB minified+gzipped so we kind of sacrifice util maintainability. OTOH we only need three tiny utils so in my book it's worth the effort.

@jdalton
Copy link
Contributor

jdalton commented Jul 23, 2015

Yap. As the old saying goes you can lead a horse to water,
but you sure as hell can't make them bust out a sweet sax solo

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