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 object-key-internal.js #1333

Closed
wants to merge 2 commits into from

Conversation

stonechoe
Copy link
Contributor

@stonechoe stonechoe commented Feb 23, 2024

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
Copy link
Contributor Author

stonechoe commented Feb 23, 2024

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.

Copy link
Owner

@zloirock zloirock left a comment

Choose a reason for hiding this comment

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

This polyfill is oriented to ES3 engines where you can't define a non-enumerable property - all actual for core-js engines that allow a definition of non-enumerable properties already have Object.keys. IIRC initially this check was here, but it was removed for the described reason.

However, it could be useful in the case of handling prototypes of built-ins. Anyway, it's not complete since this helper is also used in Object.getOwnPropertyNames.

I should think about it.

@stonechoe
Copy link
Contributor Author

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
Copy link
Owner

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

@zloirock zloirock closed this Apr 16, 2024
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.

None yet

2 participants