-
Notifications
You must be signed in to change notification settings - Fork 22.4k
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
Replaced hasOwnProperty with hasOwn in code examples #18190
Conversation
Thanks a lot! Your PR is very valuable. Before doing a formal review, thanks to your PR, I raised a question there. I think there is an extra pattern we should fix at the same time. Once it is settled, we'll move forward with this PR. |
@@ -95,7 +95,7 @@ operator on a global variable. | |||
```js | |||
'use strict'; | |||
var x = 1; | |||
globalThis.hasOwnProperty('x'); // true | |||
Object.hasOwn(globalThis, 'x') // true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed ;
that's present in all the rest code of the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I fixed it. Although there is some weird orphaned back ticks in main in line 123, which I removed.
# Conflicts: # files/en-us/web/javascript/reference/statements/var/index.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I think there is one case where we can simplify further and get rid of hasOwnProperty
/hasOwn
, and simplify even more!
@@ -450,7 +450,7 @@ Let's have a brief look at how we'd access the API using Node.js and [node-sauce | |||
myAccount.getJobs(function (err, jobs) { | |||
// Get a list of all your jobs | |||
for (let k in jobs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we couldn't use for...of
here and get one step further in simplifying this to:
for (const job of jobs) {
myAccount.showJob(job.id, function (err, res) {
let str = res.id + ": Status: " + res.status;
if (res.error) {
str += "\033[31m Error: " + res.error + " \033[0m";
}
console.log(str);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, make sense. Thanks. I implemented the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Summary
Hello, I replaced
hasOwnProperty
withhasOwn
as suggested here https://github.com/orgs/mdn/discussions/143 (number 6) in various pages.I left it in place in
en-us/web/javascript/reference/global_objects/object/hasown/index.md
anden-us/web/javascript/reference/global_objects/object/hasownproperty/index.md
as those pages explicitly mention the differences between the two functions. Some other pages already had examples with both functions so I left them untouched was well.Motivation
Improves legibility of code samples.
Supporting details
Discussion: https://github.com/orgs/mdn/discussions/143
This PR…