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: polyfill Object.hasOwn for node 14 #4152

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Conversation

ljharb
Copy link
Contributor

@ljharb ljharb commented Sep 7, 2023

@ljharb ljharb requested a review from a team as a code owner September 7, 2023 21:14
@CLAassistant
Copy link

CLAassistant commented Sep 7, 2023

CLA assistant check
All committers have signed the CLA.

@ljharb ljharb mentioned this pull request Sep 7, 2023
1 task
straker
straker previously requested changes Sep 7, 2023
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Thanks for taking on fixing < node 14.

It looks like core-js provides an implementation for Object.hasOwn, which we already depend on. We should use that one rather than bring in another dependency.

Also, the way we handle doing these types of polyfills in axe-core is to add them to the imports file and only apply the polyfill if the current implementation does not exist. That way we don't have to rewrite every use of the function and can easily remove it when we don't need the polyfill anymore.

import hasOwn from 'core-js-pure/actual/object/has-own';
 
if (!('hasOwn' in Object) {
  Object.hasOwn = hasOwn;
}

@ljharb
Copy link
Contributor Author

ljharb commented Sep 7, 2023

Sounds good, done. It seems like tests aren't running in every supported version of node, which would have caught this as well as #4127. I'm not familiar enough with circle to fix that myself, but it'd be great if someone was able to do that separately :-)

@straker
Copy link
Contributor

straker commented Sep 7, 2023

Yep, was actually looking into that myself to see what it would entail.

lib/core/imports/index.js Outdated Show resolved Hide resolved
@straker straker changed the title [Fix] Object.hasOwn does not exist in node 14 fix: polyfill Object.hasOwn does for node 14 Sep 7, 2023
@straker straker changed the title fix: polyfill Object.hasOwn does for node 14 fix: polyfill Object.hasOwn for node 14 Sep 8, 2023
@straker
Copy link
Contributor

straker commented Sep 8, 2023

Approved for security

@straker straker merged commit c7b597b into dequelabs:develop Sep 8, 2023
15 of 17 checks passed
@ljharb
Copy link
Contributor Author

ljharb commented Sep 8, 2023

Thanks! I ran npm run fmt locally and no diff was generated, so i'm not sure why the prettier task was failing.

@ljharb ljharb deleted the hasown branch September 8, 2023 17:51
@ljharb
Copy link
Contributor Author

ljharb commented Sep 8, 2023

Hopefully this will make it into v4.8.2?

@straker
Copy link
Contributor

straker commented Sep 11, 2023

Yep, I forgot I wanted this into the 4.8.1 release before we made it.

@straker
Copy link
Contributor

straker commented Sep 26, 2023

@ljharb we released this as part of axe-core v4.8.2

@ljharb
Copy link
Contributor Author

ljharb commented Sep 26, 2023

Thank you!

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.

axe-core dependency breaks nodejs14 compliancy
4 participants