-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX beta] add inspect and constructor to list of descriptor exceptions #16050
Conversation
@Dhaulagiri - I pushed some additional commits here, can you review? |
Also, FWIW, with these changes your demo repo no longer fails due to the descriptor trap (though it does have a failing tests, but I think that is intentional). |
@rwjblue correct that test failure was intentional. These changes do fix the initial issue I reported but there are additional errors that this does not address:
In this case And then
This one is more puzzling to me as it happens when I initiate a jQuery plugin and it gets to a line of code like this within jQuery (2.1.4):
|
Allow `constructor`, `prototype`, and `nodeType` accesses to return `undefined`, and add `Symbol.toStringTag` when Symbol is defined. Also, cleanup formatting to make it easier to see all the possible branches.
@Dhaulagiri - I added Also, it seems like the first one is more app specific and might actually be a correct assertion (e.g. something in the app is doing |
I'll take a closer look. It may end up being another Chai thing though as this works fine in our app but fails trying to do a deep equal comparison between two arrays with Chai. I wonder if it's trying to iterate through the keys somehow to do the comparison? It may take me a bit to find time to dig into that but the other stuff that is here will be a big help for us. |
Thanks for digging into it! We can definitely iterate on what we have here....
No problem! I'm going to land this now then, so it makes it into the next beta (making it easier to test for others also)... |
} else if ( | ||
property === 'prototype' || | ||
property === 'constructor' || | ||
property === 'nodeType' |
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.
Are these not needed for the other branch (no native Proxy
) 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.
Everything returns undefined by default in that branch, we only add assertions for the specific properties that we know about...
In addition to
inspect
I ended up needing to addconstructor
as well to get things working (confirmed locally). Let me know if there is anything else I can do here.Fixes #16049