zloirock / core-js

Standard Library
MIT License
23.99k stars 1.62k forks source link

Fix `object-key-internal.js` #1333

Closed stonechoe closed 1 month ago

stonechoe commented 3 months ago

Following code behaves differently depending on whether the native version is loaded, or if the polyfill is required and loaded:

var f = require("core-js-pure/es/object/keys");

var a = function () {};
Object.defineProperty(a, "valueOf", {value: 42, enumerable: false });

f(a).length === 0 // This should be true, but it is === 1 in polyfill

Current implementation of object-key-internal.js specially handles some buggy keys such as toString or valueOf, but it doesn't check if those keys are really enumerable, which is fixed in this PR.

stonechoe commented 3 months ago

Test failed, but it seems that an error occurred due to other factors during the process, preventing the tests from being executed at all. Could you please review this, if possible? Thank you. 😊

Edit) I triggered CI by pushing empty commit - I'm aware that adding empty commits might not fully align with your GitHub management practices. If this action contradicts your policies, I wish to apologize.

stonechoe commented 1 month ago

I didn't know that this polyfill is oriented towards ES3 engines when I made this PR. Given your explanation, it seems appropriate to close this PR; Please handle it as you see fit.

zloirock commented 1 month ago

Since anyway this logic will be removed from v4, let's close it.