no-default-id-values rule #137

fabioh8010 · 2024-11-22T21:11:41Z

This PR implements a new ESLint rule to detect and restricts usage of default number/string IDs in E/App. It's intended to be used on .eslintrc.changed.js, that will only run for the changes files of a PR.

This ESLint rule does a simple string match check to look for ID defaults in E/App, while it is not an optimal solution it should be enough for us to detect these situations.

Issue: Expensify/App#50360

Tests

Checkout this branch
Go to E/App repo and run this command to link your eslint-config-expensify repo to E/App: npx link publish <path-to-eslint-repo>/
Add the rulesdir/no-default-id-values rule to .eslintrc.changed.js and set it to error.
Make any change in a file that has plenty of examples e.g. src/libs/ReportUtils.ts, and commit that file.
Run npm run lint-changed, assert the errors were reported.

github-actions · 2024-11-22T21:12:43Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

fabioh8010 · 2024-11-28T09:15:49Z

I have read the CLA Document and I hereby sign the CLA

eslint-plugin-expensify/no-default-id-values.js

fabioh8010 · 2024-11-28T15:31:29Z

Opened PR to review cc @neil-marcellini @iwiznia @paultsimura

fabioh8010 · 2024-11-28T15:32:49Z

Should we catch assignments of default values too? e.g.

let currentUserAccountID = -1;
let transactionID = '-1';

paultsimura · 2024-11-29T09:20:28Z

Should we catch assignments of default values too? e.g.
let currentUserAccountID = -1;
let transactionID = '-1';

IMO, this is not a common case in the App, so we shouldn't worry about it. However, I'll leave the decision to @iwiznia or @neil-marcellini.

paultsimura · 2024-12-02T13:24:55Z

I'll treat the 👍 from Ionatan as approval for #137 (comment).
Reviewing the PR soon...

tgolen · 2024-12-02T15:45:09Z

@paultsimura and @fabioh8010 I believe this was correctly spotted, but the original issue isn't too clear. Empty strings are also being disallowed for defaults with this, right? I think I saw @fabioh8010 correctly spot that the danger of empty strings is subscribing to the entire collection.

fabioh8010 · 2024-12-02T19:03:23Z

@paultsimura and @fabioh8010 I believe this was correctly spotted, but the original issue isn't too clear. Empty strings are also being disallowed for defaults with this, right? I think I saw @fabioh8010 correctly spot that the danger of empty strings is subscribing to the entire collection.

That is correct, and this rule will catch these situations too 👍

paultsimura

Looks good and tests well ✔️

My only concern is the repeated "see more" link, but it'll be cluttering only until the first code cleanup. Later it'll be only useful.

paultsimura · 2024-12-03T12:56:41Z

@iwiznia, @neil-marcellini, @tgolen, please proceed with your review.

fabioh8010 · 2024-12-09T15:35:24Z

@iwiznia, @neil-marcellini, @tgolen could you have a look here? Thanks!

iwiznia

This looks good, but hard to tell if it will generate a lot of false positives, can you make this new rule run in App and see if it does?

tgolen · 2024-12-09T16:45:52Z

eslint-plugin-expensify/no-default-id-values.js

+
+        const disallowedNumberDefaults = [
+            'ID ?? -1',
+            'id ?? -1',


Rather than having all case-sensitive options, can we just make the check be cause-insensitive?

@fabioh8010 bump on this question

I would like to keep them all separated for all, just in case we discover that some string match is being problematic, but it happens to be case sensitive. Is that ok for you?

OK, yeah. That's fine. Thanks for explaining and documenting the analysis you did!

fabioh8010 · 2024-12-11T20:23:41Z

@iwiznia I analysed all search results by string match and here are the possible false-positives for each one:

Number IDs
* ID ?? -1 (201 matches)
  * src/components/ReportActionItem/IssueCardMessage.tsx 1
  * src/libs/ReportActionsUtils.ts 1
  * src/libs/actions/Card.ts 2
  * src/libs/TransactionUtils/index.ts 1
  * src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx 1
  * src/pages/settings/Wallet/Card/BaseGetPhysicalCard.tsx 1
* id ?? -1 (6 matches)
  * .github/libs/GithubUtils.ts 1
* ID ?? 0 (32 matches)
  * src/libs/PolicyUtils.ts 1
  * src/libs/actions/OnyxUpdateManager/utils/DeferredOnyxUpdates.ts 1
  * src/libs/Middleware/SaveResponseInOnyx.ts 2
* id ?? 0 (0 matches)
  * -
* ID || -1 (14 matches)
  * -
* id || -1 (0 matches)
  * -
* ID || 0 (3 matches)
  * src/libs/actions/User.ts 2
* id || 0 (0 matches)
  * -
* ID : -1 (9 matches)
  * -
* id : -1 (0 matches)
  * -
* ID : 0 (1 match)
  * -
* id : 0 (0 matches)
  * -

String IDs
*  ?? '-1' (754 matches)
   * .github/actions/javascript/getAndroidRolloutPercentage/getAndroidRolloutPercentage.ts 1
   * src/libs/DistanceRequestUtils.ts 1
   * src/libs/OptionsListUtils.ts 1
   * src/libs/PolicyUtils.ts 1
   * src/libs/WorkflowUtils.ts 2
   * src/libs/actions/Card.ts 2
   * src/libs/actions/CompanyCards.ts 1
   * src/libs/actions/TaxRate.ts 1
   * src/pages/ReportDetailsPage.tsx 2
   * src/pages/workspace/WorkspacePageWithSections.tsx 1
* ID ?? '' (183 matches)
  * src/components/MoneyRequestConfirmationList.tsx 2
  * src/components/TextPicker/TextSelectorModal.tsx 1
  * src/libs/actions/TaxRate.ts 1
  * src/libs/actions/Policy/DistanceRate.ts 4
  * src/libs/TransactionUtils/index.ts 1
  * src/pages/ReimbursementAccount/BusinessInfo/substeps/TaxIdBusiness.tsx 1
  * src/pages/settings/Wallet/PaymentMethodList.tsx 1
  * src/pages/workspace/accounting/netsuite/NetSuiteSubsidiarySelector.tsx 1
  * src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx 1
  * src/pages/workspace/distanceRates/PolicyDistanceRatesPage.tsx 5
  * src/pages/workspace/distanceRates/PolicyDistanceRatesSettingsPage.tsx 1
  * src/pages/workspace/distanceRates/PolicyDistanceRateTaxRateEditPage.tsx 1
  * src/pages/workspace/perDiem/ImportedPerDiemPage.tsx 1
  * src/pages/workspace/perDiem/WorkspacePerDiemPage.tsx 2
  * src/pages/workspace/perDiem/WorkspacePerDiemSettingsPage.tsx 1
  * src/pages/workspace/taxes/WorkspaceTaxesSettingsWorkspaceCurrency.tsx 1
* id ?? '' (17 matches)
  * src/libs/Pusher/pusher.ts 1
* ID ?? '0' (8 matches)
  * -
* id ?? '0' (0 matches)
  * -
*  || '-1' (20 matches)
   * -
* ID || '' (2 matches)
  * -
* id || '' (0 matches)
  * -
* ID || '0' (0 matches)
  * -
* id || '0' (0 matches)
  * -
*  : '-1' (38 matches)
  * -
*  : '0' (1 match)
  * -

Most of these possible false-positives are based on my own doubts if that ID could be of an entity or Onyx data, so the real number can be smaller. Anyway, I think the results here show us that we have few ones compared to the total of results of each string match.

os-botify · 2024-12-11T20:35:50Z

🚀 Published to npm in 2.0.75 🎉

iwiznia · 2024-12-11T22:33:04Z

here are the possible false-positives for each one:

So if we update the config in App, all PRs will start failing on those files?

fabioh8010 · 2024-12-12T05:34:43Z

here are the possible false-positives for each one:

So if we update the config in App, all PRs will start failing on those files?

The PRs will fail if they have changed files that have one of the matches described in the rule, it could be any .ts/.tsx file.

Initial implementation of no-default-values rule

0804d20

fabioh8010 mentioned this pull request Nov 22, 2024

[HOLD for payment 2024-12-30] [$250] Undo defaults of -1 and "-1" for account/report/policy IDs Expensify/App#50360

Open

fabioh8010 added 2 commits November 28, 2024 08:31

Refactor code and add tests

98b7777

Rename ESLint rule

b9aec0a

fabioh8010 changed the title ~~[WIP] no-default-values rule~~ [WIP] no-default-id-values rule Nov 28, 2024

CLABotify added a commit to Expensify/CLA that referenced this pull request Nov 28, 2024

@fabioh8010 has signed the CLA in Expensify/eslint-config-expensify#137

d1157c7

fabioh8010 changed the title ~~[WIP] no-default-id-values rule~~ no-default-id-values rule Nov 28, 2024

kubabutkiewicz reviewed Nov 28, 2024

View reviewed changes

eslint-plugin-expensify/no-default-id-values.js Outdated Show resolved Hide resolved

Move functions outside create()

f784aff

kubabutkiewicz approved these changes Nov 28, 2024

View reviewed changes

fabioh8010 marked this pull request as ready for review November 28, 2024 15:30

paultsimura approved these changes Dec 3, 2024

View reviewed changes

iwiznia approved these changes Dec 9, 2024

View reviewed changes

tgolen requested changes Dec 9, 2024

View reviewed changes

tgolen approved these changes Dec 11, 2024

View reviewed changes

tgolen merged commit f9e1076 into Expensify:main Dec 11, 2024
3 checks passed

fabioh8010 mentioned this pull request Dec 13, 2024

[No QA] Bump eslint-config-expensify to 2.0.75 Expensify/App#54117

Merged

50 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

no-default-id-values rule #137

no-default-id-values rule #137

fabioh8010 commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading

fabioh8010 commented Nov 28, 2024

fabioh8010 commented Nov 28, 2024

fabioh8010 commented Nov 28, 2024

paultsimura commented Nov 29, 2024

paultsimura commented Dec 2, 2024

tgolen commented Dec 2, 2024

fabioh8010 commented Dec 2, 2024

paultsimura left a comment

paultsimura commented Dec 3, 2024

fabioh8010 commented Dec 9, 2024

iwiznia left a comment

tgolen Dec 9, 2024

tgolen Dec 11, 2024

fabioh8010 Dec 11, 2024

tgolen Dec 11, 2024

fabioh8010 commented Dec 11, 2024

os-botify bot commented Dec 11, 2024

iwiznia commented Dec 11, 2024

fabioh8010 commented Dec 12, 2024 •

edited

Loading

no-default-id-values rule #137

no-default-id-values rule #137

Conversation

fabioh8010 commented Nov 22, 2024 • edited Loading

Tests

github-actions bot commented Nov 22, 2024 • edited Loading

fabioh8010 commented Nov 28, 2024

fabioh8010 commented Nov 28, 2024

fabioh8010 commented Nov 28, 2024

paultsimura commented Nov 29, 2024

paultsimura commented Dec 2, 2024

tgolen commented Dec 2, 2024

fabioh8010 commented Dec 2, 2024

paultsimura left a comment

Choose a reason for hiding this comment

paultsimura commented Dec 3, 2024

fabioh8010 commented Dec 9, 2024

iwiznia left a comment

Choose a reason for hiding this comment

tgolen Dec 9, 2024

Choose a reason for hiding this comment

tgolen Dec 11, 2024

Choose a reason for hiding this comment

fabioh8010 Dec 11, 2024

Choose a reason for hiding this comment

tgolen Dec 11, 2024

Choose a reason for hiding this comment

fabioh8010 commented Dec 11, 2024

os-botify bot commented Dec 11, 2024

iwiznia commented Dec 11, 2024

fabioh8010 commented Dec 12, 2024 • edited Loading

fabioh8010 commented Nov 22, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading

fabioh8010 commented Dec 12, 2024 •

edited

Loading