[HOLD] Destructure props of all function components by adding an ESLint rule running the linter with --fix

[hayata-suenaga]

HOLD on #20179

Per this discussion on changing the guideline to always requiring prop destructuring to assign default values to optional props.

Add the following config

'react/destructuring-assignment': ['error', 'always', {ignoreClassFields: true, destructureInSignature: 'always'}],

and run --fix

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[blazejkustra]

@hayata-suenaga Why not add the rule now in TS setup PR? If we put it in rules for TS files, we wouldn't have to run eslint --fix at all. When migrating components to TS we could remove defaultProps and use prop destructuring.

[hayata-suenaga]

I was thinking doing that but @roryabraham suggested we ran --fix before starting the migration

and if we gonna do that, we should probably do that in a separate PR

[roryabraham]

I don't feel strongly either way, but figured since the rule supports --fix we can get it out of the way now and reduce the diff of TS migration PRs.

[blazejkustra]

I was worried how defaultProps works out with destructuring props, found out it works just fine:codepen. I'm good with automatic eslint --fix in a separate PR 👌

[hayata-suenaga]

Will do this before the migration will start

[hayata-suenaga]

will put PR up tomorrow

[hayata-suenaga]

This is on hold for #20179 which will turn off an ESLint setting that conflicts with de-structured props

[hayata-suenaga]

now that #20179 has been merged, I'll work on this today

[hayata-suenaga]

Turned out that ESLint cannot fix all de-structuring rule violations automatically.

These are only files where ESLint was able to fix code automatically.

I also tried to run ESLint with the latest version (the current version specified in package.json is 7.32.0 but the latest is 8.46.0.

Current version confirmation

hayatasuenaga@Hayatas-Work-MBP App % npm list eslint --depth=0
new.expensify@1.3.50-3 /Users/hayatasuenaga/Expensidev/App
└── eslint@7.32.0

Running the latest ESLint. Still doesn't fix issues automatically

@roryabraham what do you think? should we let migration PR also handle prop de-structuring? (we can specify the rule only for ts and tsx rules so that when engineers migrate a file to a TS file, they gonna be notified about de-structuring rule violation if they didn't de-structure

[hayata-suenaga]

updated Rory about the limit of --fix option here in slack

[hayata-suenaga]

Closing this per this comment: https://expensify.slack.com/archives/C01GTK53T8Q/p1692139684018249?thread_ts=1689968108.162539&cid=C01GTK53T8Q