Skip to content

Improve .forEach examples: syntax and consistency #12799

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

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

tshemsedinov
Copy link
Contributor

Summary

  • Change var and let to const where possible
  • Use == instead of ===
  • Use one-line arrow functions is possible
  • Improve code indentation and alignment, fix other style consistency issues

Motivation

To refresh code examples before making changes to the article itself.

Metadata

  • Adds a new document
  • Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

@tshemsedinov tshemsedinov requested a review from a team as a code owner February 7, 2022 12:53
@tshemsedinov tshemsedinov requested review from sideshowbarker and removed request for a team February 7, 2022 12:53
@github-actions github-actions bot added the Content:JS JavaScript docs label Feb 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach
Title: Array.prototype.forEach()
on GitHub

No new external URLs

(this comment was updated 2022-02-07 21:15:23.935225)

@danielhjacobs
Copy link
Contributor

I think this == null is accurate based on the wording "Array.prototype.forEach called on null or undefined", since this === null won't be true for undefined. I've heard == null described as the only good reason to use non-strict equality in JavaScript. If you prefer to never use ==, that's fine, but then you should probably change to this === null || this === undefined

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks! These generally seem like good improvements but I had a couple of suggestions.


if (this == null) { throw new TypeError('Array.prototype.forEach called on null or undefined'); }

if (this === null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should delete this whole "Polyfill" section. We already link to a polyfill in "See also" and we have a general consensus that we don't want to embed polyfills in MDN pages: openwebdocs/project#27.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in separate commit, just don't cherry-pick it on landing it it should be a separate PR

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

👍 thank you @tshemsedinov !

@wbamberg wbamberg merged commit c499422 into mdn:main Feb 7, 2022
@tshemsedinov tshemsedinov deleted the patch-1 branch February 7, 2022 22:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Content:JS JavaScript docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants