Only add defined properties to the result of getOwnPropertyDescriptors #1345
Conversation
|
Hi @jordonwii, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
| body: function() { | ||
| // Adapted from: https://github.com/ljharb/test262/blob/c2eaa30b08fb1e041b7297e415b6bad8461f50dc/test/built-ins/Object/getOwnPropertyDescriptors/proxy-undefined-descriptor.js | ||
| var proxyHandler = { | ||
| getOwnPropertyDescriptor: function () {}, |
There was a problem hiding this comment.
This bug is only truly testable if ownKeys returns a key, that getOwnPropertyDescriptor returns undefined for - so it's critical to define ownKeys to return [key] (or to define the target as { a: something }), otherwise you're not testing anything.
There was a problem hiding this comment.
Got it. I misunderstood how that was being used, and removed the ownKeys bit as a last minute change. I'll add that back in.
|
Code wise looks good to me. Did you update the test accordingly? |
|
@akroshg I didn't have a chance to update it yesterday, but I just did now. I also confirmed that the test fails without my change, and passes with it. |
| assert.areEqual(undefined, descriptor, "Descriptor matches result of [[GetOwnPropertyDescriptor]] trap"); | ||
|
|
||
| var result = Object.getOwnPropertyDescriptors(proxy); | ||
| assert.isFalse(key in result, "key should not be present in result"); |
There was a problem hiding this comment.
The in operator also returns true for properties found in the prototype change. Here we don't expect the prototype change of a normal plain object to have a property named 'a', but it would be better practice to use obj.hasOwnProperty(key) instead
|
Looks great. One comment on the test that I think should be addressed, otherwise good to go. |
|
Thanks @jordonwii |
Fixes #1342
For the tests for this change, I adapted @ljharb's tests from the discussion he linked to in the issue and trimmed it down to test just this change.