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

Faster/simpler isPlainObject check #3483

Merged
merged 3 commits into from
Nov 8, 2017
Merged

Faster/simpler isPlainObject check #3483

merged 3 commits into from
Nov 8, 2017

Conversation

timdorr
Copy link
Contributor

@timdorr timdorr commented Nov 8, 2017

This runs up the prototype chain to check equivalency. When run in a cross-realm environment (differing contexts, iframes, etc), this ensures we're checking the value's prototype matches its context-specific instance of Object.

This is faster than calling toString() on the constructor. There's still the baseGetTag() call, which does its own toString(), but this swaps out one for cross-realm stuff. It's also a bit simpler to understand, I think.

This runs up the prototype chain to check equivalency. When run in a cross-realm environment (differing contexts, iframes, etc), this ensures we're checking the value's prototype matches its context-specific instance of Object. 

This is faster than calling `toString()` on the constructor. There's still the `baseGetTag()` call, which does its own `toString()`, but this swaps out one for cross-realm stuff. It's also a bit simpler to understand, I think.
@jsf-clabot
Copy link

jsf-clabot commented Nov 8, 2017

CLA assistant check
All committers have signed the CLA.

@timdorr
Copy link
Contributor Author

timdorr commented Nov 8, 2017

Do I need to do anything to get the tests to run? I checked out the 3203 branch and ran them there successfully.

@jdalton
Copy link
Member

jdalton commented Nov 8, 2017

Do I need to do anything to get the tests to run? I checked out the 3203 branch and ran them there successfully.

Awesome! Thank you!

That's fine for now. I think we can even remove some of the baseTag checks too.

@timdorr
Copy link
Contributor Author

timdorr commented Nov 8, 2017

I think we can even remove some of the baseTag checks too.

I had some difficulty there. These two tests fail in that case:

should return `false` for non-Object objects
should return `false` for objects with a read-only `Symbol.toStringTag` property

There's some edge case handling in #2705, but that's a good issue to read through for a reminder of why this check exists.

It does feel a little weird to be looking to toString() to answer the question. I'm not sure if there's a workaround, but with all these new reflective APIs to override the toString() output, it's definitely a complex problem.

@jdalton
Copy link
Member

jdalton commented Nov 8, 2017

Cool. I'll re-evaluate those checks for v5.

@lock
Copy link

lock bot commented Dec 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants