[No QA] Add ESLint rule banning runtime react-native-onyx/dist/OnyxUtils imports - #100534
[No QA] Add ESLint rule banning runtime react-native-onyx/dist/OnyxUtils imports#100534MelvinBot wants to merge 4 commits into
Conversation
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Android test steps verification results: ✅ pass Standalone Android NewDot loads to Home/Inbox and renders normally; no JS errors, red-box screens, or crashes were observed in app logs; basic chat navigation (open report, back to Inbox) works as expected. This is consistent with this PR being an ESLint-config-only change with no runtime impact — the PR's
One caveat worth stating plainly: this ran against the standalone Android NewDot build, not a build of this branch. That is acceptable here because the diff only touches ESLint config and comments, so it cannot change runtime behavior. The authoritative check for this PR is full-repo lint in CI, not device testing. |
|
🤖 Web test steps verification results: App loads to Home/Inbox and basic chat navigation works cleanly on dev NewDot web. The one documented check — "Verify that no errors appear in the JS console" — could not be directly proven, because the web tooling exposes no verb for reading the browser console; no errors surfaced via HTTP status codes or in-app error UI, but that is indirect evidence rather than a read of the console itself.
Two caveats worth stating plainly:
The console checkbox still needs a human to tick — open DevTools on this branch and confirm the console is clean. |
This comment was marked as outdated.
This comment was marked as outdated.
|
|
This comment was marked as outdated.
This comment was marked as outdated.
|
|
This comment was marked as outdated.
This comment was marked as outdated.
|
|
|
@MelvinBot Can we use |
|
|
|
@MelvinBot Can we use |
|
|
|
@MelvinBot Can we use |
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Yes — done in You were right and I had it backwards. What the diff looks like now
Net:
The |
|
@MelvinBot Please merge main and resolve conflicts |
…tilsRuntimeImport Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com> # Conflicts: # config/eslint/eslint.seatbelt.tsv
|
Merged One conflict, in Resolved by taking VerificationThe in-repo ratchet also fixed the thing I flagged last time:
Only |
| // `OnyxUtils` lives in `react-native-onyx/dist/*`, which is package internals rather than the public API. | ||
| // This uses the typescript-eslint variant of `no-restricted-imports` (a separate rule name, so it stacks | ||
| // with the base rule above instead of overriding it) because only that variant supports `allowTypeImports`, | ||
| // and `src/types/onyx/Request.ts` legitimately does `import type OnyxUtils from 'react-native-onyx/dist/OnyxUtils'`. | ||
| // The 14 existing runtime imports are grandfathered via the seatbelt baseline so they stay visible and can be | ||
| // paid down under https://github.com/Expensify/App/issues/98610; this only blocks NEW usages. |
There was a problem hiding this comment.
@MelvinBot I don’t think we need these comments. Please remove them.
There was a problem hiding this comment.
Removed — a1abffb. All six comment lines are gone; the config block itself is unchanged.
The one thing those comments were carrying was why this uses @typescript-eslint/no-restricted-imports rather than adding an entry to the shared restrictedImportPaths array — allowTypeImports only exists on the typescript-eslint variant, and src/types/onyx/Request.ts needs it. That rationale is still written up in the PR description, so it isn't lost.
CI on the new commit: ESLint check ✅ (7m), Oxfmt ✅, Spell check ✅, Knip ✅.
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
@MelvinBot Please make the PR ready for review! |
|
@linhvovan29546 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
This PR adds a new |
|
Done — this PR is out of draft and ready for review. Good news on the seatbelt switch: Two things still need a human before merge:
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
|
@MelvinBot Please add the No QA tag to the PR title. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1abffb399
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
|
|
||
| { | ||
| files: ['**/*.ts', '**/*.tsx'], |
There was a problem hiding this comment.
Because this block matches only .ts and .tsx, linted runtime JavaScript such as the app entry point index.js can still statically import react-native-onyx/dist/OnyxUtils without triggering the new rule. This leaves the intended repository-wide boundary incomplete for supported source types; include the linted JS extensions (.js, .jsx, .mjs, and .cjs) or add an equivalent core-rule block for them.
Useful? React with 👍 / 👎.
MelvinBot
changed the title
|
Done — the title is now Also closing the loop on my last comment: |
| "../../src/libs/Middleware/Logging.ts" "@typescript-eslint/no-unsafe-type-assertion" 1 | ||
| "../../src/libs/Middleware/Pagination.ts" "@typescript-eslint/no-unsafe-type-assertion" 5 | ||
| "../../src/libs/Middleware/SaveResponseInOnyx.ts" "no-restricted-syntax" 1 | ||
| "../../src/libs/migrations/MoveFilesOutOfDocuments/index.native.ts" "@typescript-eslint/no-restricted-imports" 1 |
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
Thanks for working on this. Hopefully it'll stop the spread of Onyx.get throughout app.
4 participants
Explanation of Change
OnyxUtilsis imported from the internal deep pathreact-native-onyx/dist/OnyxUtils, which is package internals rather than the public API. The only sanctioned ways to read Onyx data areuseOnyx()(render path) andOnyx.connectWithoutView()(non-render logic). This PR adds the lint boundary so the deep import can't be reintroduced.What changed:
config/eslint/eslint.config.mjs— new flat-config block for**/*.ts/**/*.tsxthat bans thereact-native-onyx/dist/OnyxUtilsmodule path.Two implementation details worth reviewing:
@typescript-eslint/no-restricted-imports, not the baseno-restricted-importsthat the sharedrestrictedImportPathsarray feeds. The issue's suggested snippet usedallowTypeImports: true, but that option only exists on the typescript-eslint variant — the base rule's schema rejects it, so putting the entry inrestrictedImportPathswould either fail config validation or break the legitimate type-only import insrc/types/onyx/Request.ts.allowTypeImportsis what keepsimport type OnyxUtils from 'react-native-onyx/dist/OnyxUtils'passing.no-restricted-importsblocks instead of overriding any of them. No existing restriction is dropped.config/eslint/eslint.seatbelt.tsv— the 14 existing runtime imports are grandfathered into the seatbelt baseline (one entry per file, count1), following the "I'm enabling a new rule repo-wide" workflow inLINTING.md. The rule lands now and blocks any new usage, while the existing call sites stay visible in the baseline so they can be paid down under this issue:src/libs/migrations/MoveFilesOutOfDocuments/index.native.ts— the runtimeOnyxUtils.get()call that landed after this issue was written.tests/(tests/is linted; it is not inglobalIgnores).An earlier revision of this PR used 14
eslint-disable-next-linecomments instead. That was the wrong tool: perLINTING.md,eslint-disableis for permanent exceptions, and it hides violations from the baseline entirely. These suppressions are temporary — they exist only until the call sites are removed — so the seatbelt baseline is the correct escape hatch. Switched atlinhvovan29546's request.Aside from the baseline rows, no source or test file differs from
main.Fixed Issues
$ #98610
PROPOSAL:
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
This is a lint-config-only change; there is no runtime behavior change to exercise in the app.
Offline tests
N/A — lint configuration change only, no runtime behavior change.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
No QA — this change only affects lint rules and the seatbelt baseline. No shipped code paths change.
AI Tests
Run locally by MelvinBot on the current head:
npm run linton all 15 changed files — 0 errors. Confirms the rule fires and that every baseline row matches its real count.1.npm run lint-changed— clean.npm run spell-changed— 1 file checked, 0 issues.react-native-onyx/dist/OnyxUtils— 14 runtime imports (all baselined), 1 type-only import (allowed byallowTypeImports), and the config entry itself. Nothing else in the repo can trip the new rule.ESLint checkin CI — passed (6m16s) on the merge commit. This is the authoritative check for the baseline.npm run typecheck,npm test,npm run react-compiler-compliance-check— skipped. No.ts/.tsxfile differs frommain; the only diff is the ESLint config block and the TSV baseline rows.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari