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

Modernize JS in Web/API: Use for...of when possible #18103

Closed
teoli2003 opened this issue Jul 8, 2022 · 6 comments
Closed

Modernize JS in Web/API: Use for...of when possible #18103

teoli2003 opened this issue Jul 8, 2022 · 6 comments
Labels
Content:WebAPI Web API docs

Comments

@teoli2003
Copy link
Contributor

teoli2003 commented Jul 8, 2022

We are modernizing our code examples in JS.

We would like to use for...of whenever possible, in lieu of for (;;).

This is possible when looping through all the elements of an array:

for (let i=0; i< arrayVar.length; i++) {
  [ uses arrayVar[i] ]

should become

for (const arrayValue of arrayVar) {
  [ uses arrayValue ]

Note: the arrayValue should be tailored to each case. For example, if arrayVar is cars, arrayValue should be car.

I've created a spreadsheet to help coordinate the different writers. Try to group your changes by APIs, or, when updating unrelated pages, put around 10-15 pages in a PR (not less, it is too fragmented, not more, it becomes difficult to review). Don't overcommit, let everybody have their share.

  • Write your name when you start. Next to each file (max. 10 at a time)
  • Link to the PR when in
  • Mark the lines done when it is merged.

You can take a new batch when your PR is created, no need to wait for it to be merged (except the first time, you don't want to redo the work because of a misunderstanding!)

For your first PR, start with one file only so it is easy to check you have understood.

A few extra tips:

  1. Mark your PR as a draft (using GitHub UI), if it is not ready for review.
  2. Choose a useful title, with the API name for example.
  3. Link to this bug in the PR description using Part of Modernize JS in Web/API: Use for...of when possible #18103.

Feel free to ask any question here. Thanks in advance.

@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Jul 8, 2022
@teoli2003 teoli2003 mentioned this issue Jul 8, 2022
3 tasks
@Josh-Cena Josh-Cena added Content:WebAPI Web API docs Content:JS JavaScript docs and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. Content:JS JavaScript docs labels Jul 8, 2022
@ghost
Copy link

ghost commented Jul 13, 2022

Are you open to using iterators besides the implicit [Symbol.iterator], e.g., Array#keys()?

// increment each element
for (let index = 0; index < sequence.length; ++index) {
 sequence[index] += 1;
}

->

// increment each element
for (const index of sequence.keys()) {
 sequence[index] += 1;
}

(no, this isn't a snippet from from MDN)

@ghost
Copy link

ghost commented Jul 13, 2022

And I believe that I've encountered for in loops on MDN, are we interested in rewriting those to use Object.{keys values}? Although, it seems that by the description, this issue is scoped to reducing for (...; ...; ...)?

@teoli2003
Copy link
Contributor Author

The list I created contains only for(;;) only because of the way I built it (looking for for(;;) containing the word length). This leads to false positives but also false negatives.

For for...in, we should fix them when it simplifies. There are clearly cases where we had for...in followed by hasOwn/hasOwnProperties that can be simplified with for...of. I'm open to simplifying more cases: the criterion here is it easier to read. Do you have an example?

@ghost
Copy link

ghost commented Jul 13, 2022

For for...in, we should fix them when it simplifies. There are clearly cases where we had for...in followed by hasOwn/hasOwnProperties that can be simplified with for...of. I'm open to simplifying more cases: the criterion here is it easier to read. Do you have an example?

No, that was specifically what I had in mind: particularly for ... in object followed by object.hasOwnProperties, although I hadn't searched for any specific uses.

If I find time, I may help out with a more thorough search, and extend this to more cases.

@teoli2003
Copy link
Contributor Author

We fixed one of these for...in followed by hasOwnProperties yesterday (with a for...of). There were others, but they needed the index, so we kept the for...in, although we likely could do a larger rewrite to get rid of it too.

@teoli2003
Copy link
Contributor Author

This is mostly done. There may a few cases here or there, but no need to keep this open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

No branches or pull requests

2 participants