close
The Wayback Machine - https://web.archive.org/web/20220124163631/https://github.com/nodejs/node/pull/41664
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

Jump to bottom

test: use Object.hasOwn() where applicable #41664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

test: use Object.hasOwn() where applicable #41664

wants to merge 1 commit into from
+40 −40

Conversation

@Trott
Copy link
Member

@Trott Trott commented Jan 23, 2022

Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.
@Trott
test: use Object.hasOwn() where applicable
797c71b 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 23, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42095/

benjamingr
benjamingr approved these changes Jan 23, 2022
View changes
Copy link
Member

@benjamingr benjamingr left a comment

Sure, but why? I get the value of hasOwn when interacting with objects whose prototype you can't be certain of but these are our own tests

aduh95
aduh95 approved these changes Jan 23, 2022
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment

The goal is to enforce it with a lint rule in the future? I'm all for it.

@Trott
Copy link
Member Author

@Trott Trott commented Jan 23, 2022 •
edited

Sure, but why? I get the value of hasOwn when interacting with objects whose prototype you can't be certain of but these are our own tests

The goal is to enforce it with a lint rule in the future? I'm all for it.

These are all flagged by the no-prototype-builtins rule which is included in the eslint:recommended configuration and which we then explicitly disable. I would like to remove that exception and enable the rule. These account for most of the code flagged, but there are still a few others.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 23, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42106/

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 23, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42111/

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 23, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/42114/

@github-actions github-actions bot mentioned this pull request Jan 24, 2022
Open
22 tasks
bnb
bnb approved these changes Jan 24, 2022
View changes
Copy link
Member

@bnb bnb left a comment

:))))))))))))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnb bnb

@addaleax addaleax

@benjamingr benjamingr

@aduh95 aduh95

@RaisinTen RaisinTen

@Mesteery Mesteery

Assignees
No one assigned
Labels
dont-land-on-v12.x dont-land-on-v14.x
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
8 participants
@Trott @nodejs-github-bot @bnb @addaleax @benjamingr @aduh95 @RaisinTen @Mesteery