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

refactor: use prototype #2165

Merged
merged 5 commits into from May 16, 2022
Merged

refactor: use prototype #2165

merged 5 commits into from May 16, 2022

Conversation

jly36963
Copy link
Contributor

Relates to: #2161 (comment)

@jly36963 jly36963 requested a review from bcoe April 11, 2022 22:43
@bcoe bcoe changed the title fix: use prototype refactor: use prototype Apr 21, 2022
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

MDN suggests using Object.hasOwn, vs., Object.hasOwnProperty, or Object.prototype.hasOwnProperty:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty#using_hasownproperty_as_a_property_name

Should we go this route?

@bcoe
Copy link
Member

bcoe commented Apr 21, 2022

CC: @dhensby ☝️

@dhensby
Copy link

dhensby commented Apr 21, 2022

Object.hasOwn() is only in node >= 16.9, so that's a call for this library to make. As the MDN docs say, this implementation (using the Object prototype) is acceptable

@jly36963
Copy link
Contributor Author

@bcoe The second code block in the MDN fragment you linked recommends Object.prototype.hasOwnProperty.call as an acceptable alternative, and as @dhensby said, Object.hasOwn is a relatively new feature in Node.

@bcoe
Copy link
Member

bcoe commented Apr 25, 2022

👏 works for me, I should have read when hasOwn was introduced.

@bcoe bcoe merged commit 8912078 into yargs:main May 16, 2022
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

3 participants