Skip to content

Conversation

@TkDodo
Copy link
Collaborator

@TkDodo TkDodo commented May 12, 2025

While it’s legal to iterate over an array with for-in, it’s likely not intended (for-in iterates over the properties of an Object) because it will return the index as a string, and has other room for bugs as stated here.

Moving to for-of is a better fix in many places, as it will also rid us of no-null assertions when doing an indexed access.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 12, 2025
if (shouldDelete) {
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
delete errorMessages[errorKey];
delete errorMessages[errorKey as keyof typeof errorMessages];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: I’m pretty sure this can’t have worked before. If we iterate with:

for (const errorKey in Object.keys(errorMessages))

errorKey will be the stringified index of the keys array, so ['0', '1'] etc. Then, going to the actual errorMessages object and deleting something at '0' doesn't do anything unless the key is literally the string '0'.

Maybe the intention was to do:

for (const errorKey in errorMessages)

to iterate over all keys?

@roggenkemper am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

good catch! i guess the case that it covered wasn't common enough to come up often.

@TkDodo TkDodo marked this pull request as ready for review May 12, 2025 09:35
@TkDodo TkDodo requested review from a team as code owners May 12, 2025 09:35
@TkDodo TkDodo requested a review from a team May 12, 2025 09:35
Copy link
Member

@JonasBa JonasBa left a comment

Choose a reason for hiding this comment

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

Nice to see this might have also caught a bug in that loop. Approving the PR (provided @roggenkemper confirms this fixes a bug)

@codecov
Copy link

codecov bot commented May 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #91420      +/-   ##
==========================================
- Coverage   87.59%   87.59%   -0.01%     
==========================================
  Files       10326    10326              
  Lines      586114   586100      -14     
  Branches    22618    22618              
==========================================
- Hits       513417   513403      -14     
  Misses      72278    72278              
  Partials      419      419              

@TkDodo TkDodo merged commit 4490172 into master May 13, 2025
48 of 50 checks passed
@TkDodo TkDodo deleted the tkdodo/ref/no-for-in-array branch May 13, 2025 08:58
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants