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

@jly36963 jly36963 commented Apr 11, 2022

Relates to: #2161 (comment)

@jly36963 jly36963 requested a review from bcoe Apr 11, 2022
@bcoe bcoe changed the title fix: use prototype refactor: use prototype Apr 21, 2022
bcoe
bcoe approved these changes Apr 21, 2022
Copy link
Member

@bcoe bcoe left a comment

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 bcoe commented Apr 21, 2022

CC: @dhensby ☝️

@dhensby
Copy link

@dhensby 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

@jly36963 jly36963 commented Apr 21, 2022

@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
bcoe approved these changes Apr 25, 2022
@bcoe
Copy link
Member

@bcoe 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
7 checks passed
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