diff --git a/.claude/skills/app-coding-standards/SKILL.md b/.claude/skills/app-coding-standards/SKILL.md index c87d2d2d8714..208468fd527d 100644 --- a/.claude/skills/app-coding-standards/SKILL.md +++ b/.claude/skills/app-coding-standards/SKILL.md @@ -81,6 +81,9 @@ Coding standards for the Expensify App. Each standard is a standalone file in `r - [UI-4](rules/ui-4-layout-spacing-tokens.md) — Type and responsive insets come from tokens - [UI-5](rules/ui-5-reuse-existing-styles.md) — Reuse an existing style or StyleUtils helper instead of adding a new one +### Onyx +- [ONYX-1](rules/onyx-1-no-render-reachable-onyx-read.md) — Keep Onyx reads off the render path and out of a written tick + ## Usage **During development**: When writing or modifying `src/` files, consult the relevant standard files for detailed conditions, examples, and exceptions. diff --git a/.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md b/.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md new file mode 100644 index 000000000000..70067ce7c226 --- /dev/null +++ b/.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md @@ -0,0 +1,193 @@ +--- +ruleId: ONYX-1 +title: Keep Onyx reads off the render path and out of a written tick +--- + +## [ONYX-1] Keep Onyx reads off the render path and out of a written tick + +### Reasoning + +`await Onyx.get()` reads a key once and never subscribes. `Onyx.multiGet()` does the same for each key in an array, so everything below about `Onyx.get` applies to it too. `no-unsafe-onyx-read` catches most misuse, so this rule covers only what lint can't see. + +Do not re-check these: + +| Enforced | By | +|---|---| +| `Onyx.get` or `Onyx.multiGet` outside `src/components`, `src/pages`, `src/hooks` and `tests` | `no-unsafe-onyx-read` | +| A read written in a component or hook body (including `memo` and `forwardRef` wrappers and any function with a top-level `return `), in a JSX expression, in a `useMemo` callback, a `selector`, a lazy initializer or a `useSyncExternalStore` snapshot, in a same-file function passed by name to a `useMemo`, `useState`, `useReducer` or `useSyncExternalStore` slot, to `memo` or `forwardRef`, or to a `selector`, in a same-file helper that render or module scope calls, or at module scope. Not a helper imported from another file, see A | `no-unsafe-onyx-read` | +| A read inside an effect, or in a same-file function an effect calls | `no-unsafe-onyx-read` | +| A read in a module-level function, object or class in `src/components`, `src/pages` or `src/hooks`, outside a component or hook body | `no-unsafe-onyx-read` | +| A reference to `Onyx.get` or `Onyx.multiGet` that is not a direct call (`.call`, `.bind`, `keys.map(Onyx.get)`, `const read = Onyx.get`, `const {get} = Onyx`) | `no-unsafe-onyx-read` | +| The `Onyx` object used as a value instead of through a member, such as `{...Onyx}` or `const api = Onyx` | `no-unsafe-onyx-read` | +| A Search snapshot key, or a key lint can't resolve, including any `multiGet` element | `no-onyx-get-snapshot-key` | +| A runtime import of `react-native-onyx/dist/OnyxUtils` | `@typescript-eslint/no-restricted-imports` | +| An inline `eslint-disable` that covers `no-unsafe-onyx-read`, named or blanket, or one over a runtime OnyxUtils import, in any linted file outside `tests/` | `scripts/checkOnyxLintBypass.ts` | +| An `eslint-disable` of `no-onyx-get-snapshot-key` with a reason under 12 letters or digits after `--` or in block form, in any linted file outside `tests/` | `scripts/checkOnyxLintDisableReason.ts` | +| A missing `await` whose value is then used | `tsc` | + +A disable of `no-onyx-get-snapshot-key` is allowed with a reason, and the script checks only that it holds at least 12 letters or digits. Judge its content under E1. The key `${ONYXKEYS.COLLECTION.REPORT}${id}`, inline or through a const, passes `no-onyx-get-snapshot-key` without a disable when `id` is a same-file `const` bound to `ONYXKEYS.CONCIERGE_REPORT_ID` by `await Onyx.get(...)`, `const [id] = useOnyx(...)` with no `selector`, or the matching slot of `Onyx.multiGet(...)`. That read needs no E1 review, and A through D still apply to it. + +What lint leaves to review is below: a read reached from render across files (A), write ordering (B), a handler an effect in another file calls (C), a read value shown later as current (D), and the content of a disable reason (E). + +Mutating a read result writes the cache, since a single-key read returns the cached object and a collection read returns the cached members. `useOnyx` hands out the same object, so this stays a documented convention and isn't flagged here. A `set` with that same object can be skipped as unchanged, so the mutation is neither saved nor announced. + +**A. Position.** A read runs during render when any render body reaches it, whichever file holds it. Lint counts a function as a render body when it's named like a component or hook, has a top-level `return `, is wrapped in `memo` or `forwardRef`, or is passed to a `useMemo`, `useState`, `useReducer` or `useSyncExternalStore` slot or a `selector` option, inline or by name. It follows a helper the render body calls in the same file, through aliases and `useCallback`, but misses one imported from another file. It also misses a helper that returns JSX from inside an `if` or `switch`, a component passed inline to any call other than `memo` or `forwardRef` (such as a HOC), and a function passed to a child that calls it during render. When one of these helpers is `async`, `tsc` catches it only where the Promise lands in a slot typed `ReactElement`, such as `renderItem`. A JSX child or a `render*` prop typed `ReactNode` accepts a Promise, so rely on `tsc` only when it reports an error. + +**B. Tick.** Awaiting a read doesn't wait for an earlier write. `Onyx.get` captures the cached value when it's called. `set` and `multiSet` update the cache before they return, but `merge`, `mergeCollection`, `setCollection`, `clear` and every `update` operation, including SET, apply to the cache after at least one microtask. A read queued behind them sees the old value, so await the write or read first. A derived key (`ONYXKEYS.DERIVED.*`) is recomputed on a microtask after its source changes, so a read in the same synchronous stretch returns the old derived value, even after a `set` that already updated the source. Read a derived key only before writing its sources. + +**C. Effect in another file.** Lint bans a read inside an effect, but only within one file. A handler that reads can still end up in an effect when it's passed to a child or hook that calls it from `useEffect`, `useLayoutEffect` or `useFocusEffect`. A receiver that only registers the handler for an event (an `on*` prop, `addEventListener`, `useKeyboardShortcut`) is fine, even if the handler sits in that effect's dependency array. + +**D. Output.** A read value that reaches the screen later, through state, a ref or a module variable a component renders, stays frozen at the moment of the read. Flag it when the screen presents it as the current value. + +### Incorrect + +**A1. A hook calls a reading helper from render.** + +```ts +// src/hooks/useCurrentUserEmail/getCurrentUserEmail.ts +export default async function getCurrentUserEmail() { + return (await Onyx.get(ONYXKEYS.SESSION))?.email; // fine on its own, and lint only sees this file +} + +// src/hooks/useCurrentUserEmail/index.ts +function useCurrentUserEmail() { + return getCurrentUserEmail(); // render gets a Promise, and tsc accepts it +} +``` + +**A2. A render body lint doesn't recognize.** + +```tsx +// The JSX return sits inside a switch, so lint sees no top-level JSX return and no render body. +async function renderTagBadge(policyID: string) { + const tags = await Onyx.get(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`); + + switch (Object.keys(tags ?? {}).length) { + case 0: + return null; + default: + return ; + } +} + +// A render prop typed ReactNode accepts the Promise, and lint sees only a callback passed as a prop +
renderTagBadge(policyID)} />; +``` + +**B. A read right after a write.** + +```tsx +const onSave = async () => { + Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: true}); + const account = await Onyx.get(ONYXKEYS.ACCOUNT); // isLoading is still the old value +}; +``` + +**C. A child calls the handler from an effect.** + +```tsx +// src/pages/ContactsPage.tsx +function ContactsPage() { + const importContacts = async () => saveContacts(await Onyx.get(ONYXKEYS.COUNTRY_CODE)); + return ; +} + +// src/components/ContactsList.tsx +function ContactsList({onReady}: Props) { + useEffect(() => { + onReady(); // the read now runs inside this effect, and lint only checks one file + }, [onReady]); +} +``` + +**D. A value shown as current, captured at a tap.** + +```tsx +function CurrentTheme() { + const [theme, setTheme] = useState(); + const onPress = async () => setTheme(await Onyx.get(ONYXKEYS.PREFERRED_THEME)); + + // Change the theme in Settings and this still shows the old one. + return Current theme: {theme}; +} +``` + +### Correct + +```tsx +// A: the hook subscribes. +function useCurrentUserEmail() { + const [email] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.email}); + return email; +} + +// B: await the write, or read before it. +const onSave = async () => { + await Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: true}); + const account = await Onyx.get(ONYXKEYS.ACCOUNT); +}; + +// B, derived: read the derived key before writing its source. +const onCloseCard = async (cardID: string) => { + const cards = await Onyx.get(ONYXKEYS.DERIVED.NON_PERSONAL_AND_WORKSPACE_CARD_LIST); + Onyx.merge(ONYXKEYS.CARD_LIST, {[cardID]: {state: CONST.EXPENSIFY_CARD.STATE.CLOSED}}); +}; + +// C: keep the subscription in the parent and pass the value down. +const [countryCode] = useOnyx(ONYXKEYS.COUNTRY_CODE); +return saveContacts(countryCode)} />; + +// D: anything shown as current stays on useOnyx. +const [theme] = useOnyx(ONYXKEYS.PREFERRED_THEME); +return Current theme: {theme}; +``` + +--- + +### Review Metadata + +#### A. Position + +- A1. The diff adds a read to a function that some caller reaches from render: a component or hook body, a `useMemo` callback, a `useOnyx` selector, a lazy initializer, an IIFE or array callback in the body, or a local function the body calls. Grep `src/` for the function's name, ignoring imports. If a caller is a plain function, repeat the Grep on its name. When the render-side call is in the diff, comment on that call and name the read. Otherwise, comment on the read and name the calling file and line. A3 covers a new call whose value is discarded. A1 covers every other render-reached read. +- A2. The function holding the read returns JSX from any branch, or is passed as `renderItem`, `ListHeaderComponent`, or any `render*` or `*Component` prop. Flag the read. +- A3. The diff adds a call at a render position in a component or hook, the call's value is discarded or the callee returns `void`, and the callee's file contains `Onyx.get` or `Onyx.multiGet`. Comment on the call. +- A4. The diff passes a function that reads, or whose file contains `Onyx.get` or `Onyx.multiGet`, as a prop, and the receiver calls that prop from render. Open the receiver's file and Grep the prop's name followed by `(`. Follow forwarded props. If the receiver can't be resolved (a spread, or a component held in a variable), ask the author to confirm nothing calls it during render. + +#### B. Tick + +- B1. An `Onyx.merge`, `Onyx.mergeCollection`, `Onyx.setCollection`, `Onyx.update` or `Onyx.clear` that isn't awaited is followed in the same tick by a read of the written key, a member of the written collection, or a `DERIVED` key built from either (see `dependencies` in `src/libs/actions/OnyxDerived/configs/`). `Onyx.set` and `Onyx.multiSet` update the cache before they return, so a read after them is fine, but a `DERIVED` key still lags. The read can be direct or in a called function whose file contains it. Repeat on any plain function it calls. Comment on the write, naming the callee and the key. +- B2. The diff adds a read to a function whose callers write that key before calling it in the same tick. + +#### C. Effect in another file + +- C1. The diff passes a function that reads, or whose file contains `Onyx.get` or `Onyx.multiGet`, to another component or hook, or turns a function already passed that way into one that reads. Open the receiver, Grep the prop's name followed by `(`, and flag when a call sits inside a `useEffect`, `useLayoutEffect` or `useFocusEffect` callback, directly or through a local function. Follow forwarded props. The prop's name doesn't matter: an `onReady` the receiver calls from an effect is a finding. Comment on the prop, naming the receiver's effect. + +#### D. Output + +- D1. The read's value goes to a `useState` setter, a `useRef`, or a module variable, a render position in the same file reads it, and the screen presents it as the current value. Comment on the read. + +#### E. Reason + +- E1. The diff adds or edits an `eslint-disable` of `no-onyx-get-snapshot-key`. The text after `--` must say why that key never went through the `useOnyx` snapshot redirection: it replaces a module-level `Onyx.connect()` used only by handlers, or the value is never read inside a Search scope. Flag a reason that restates the rule, says live data is wanted without saying why the snapshot was never involved, or names a different key than the one read. Comment on the directive. + +**DO NOT flag if:** + +- The read sits in an event handler or `useCallback` body that render doesn't invoke, the reading function isn't exported, isn't a render body by A2, and isn't passed as a render callback, and no B, C, D or E item applies +- The prop holding the reader is named `on*` or `handle*`, and every receiver attaches it to an event or calls it from a handler. A receiver that calls it from an effect is C1 regardless of the name +- The value only reaches a handler argument or a request field and is never rendered +- The receiver only registers the handler for an event (an `on*` prop, `addEventListener`, `useKeyboardShortcut`), even if the handler is in an effect's dependency array +- The removed `useOnyx` value appears nowhere in the diff except the converted call's arguments +- The value is meant as a point-in-time value taken at the event, and nothing downstream expects it to update +- The write is awaited, or the read runs in its `.then`, and the read key isn't derived from the written one +- (B only) The read sits in a deliberate deferral: a `.then`, a timer, `runAfterTransitions`, `runAfterInteractions`, or a callback passed to an async API. Don't suggest hoisting it above the deferral, since that pins the value to the moment before the wait. A `.then` chained on the read itself is not a deferral, and a deferral never excuses an A, C or D finding +- The write and the read are in exclusive branches, or the write's branch returns first +- The keys differ, and the read key isn't derived from the written one and isn't a member of the written collection + +**Search Patterns** (hints for reviewers): + +- `Onyx.get(`, `Onyx.multiGet(` +- `Onyx.merge(`, `Onyx.update(`, `Onyx.set(`, `Onyx.mergeCollection(` +- `ONYXKEYS.DERIVED` +- removed `useOnyx(` lines in the diff, then that variable's name in the rest of the diff +- `useEffect(`, `useLayoutEffect(`, `useFocusEffect(`, `useRef(`, `useState(` +- `runAfterTransitions`, `runAfterInteractions`, `.then(`, `setTimeout(` around a read that follows a write diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 72cbf457423c..c7641cc0817e 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -51,7 +51,10 @@ jobs: # (espree/acorn) whose bumps can silently change parse results, plus # typescript/ts-api-utils since a TS bump can change type-aware lint results. # The cache key hashes this slice so a cold full re-lint happens only when - # the lint config or a lint dependency changes -- not on every dep bump. + # the lint config, a local rule or a lint dependency changes, not on every dep bump. + # The local rules are in the key because ESLint's own cache hashes the serialized + # config, which leaves out rule functions, so editing a rule alone never invalidates it. + # no-onyx-get-snapshot-key also loads SnapshotUtils.ts and runtimeDefaults.ts at lint time. - name: Extract lint-related deps from package-lock.json shell: bash run: | @@ -70,9 +73,9 @@ jobs: uses: actions/cache/restore@9255dc7a253b0ccc959486e2bca901246202afeb with: path: node_modules/.cache/eslint - key: ${{ runner.os }}-eslint-${{ hashFiles('eslint.config.mjs', 'config/eslint/**', 'lint-deps-lock.json') }}-${{ github.sha }} + key: ${{ runner.os }}-eslint-${{ hashFiles('eslint.config.mjs', 'config/eslint/**', 'eslint-plugin-local-rules/**', 'src/libs/SnapshotUtils.ts', 'src/CONST/runtimeDefaults.ts', 'lint-deps-lock.json') }}-${{ github.sha }} restore-keys: | - ${{ runner.os }}-eslint-${{ hashFiles('eslint.config.mjs', 'config/eslint/**', 'lint-deps-lock.json') }}- + ${{ runner.os }}-eslint-${{ hashFiles('eslint.config.mjs', 'config/eslint/**', 'eslint-plugin-local-rules/**', 'src/libs/SnapshotUtils.ts', 'src/CONST/runtimeDefaults.ts', 'lint-deps-lock.json') }}- # ESLint's cache doesn't track cross-file TypeScript dependencies, which can cause stale errors. # If lint fails, we clear the cache and retry to rule out false positives. @@ -96,7 +99,7 @@ jobs: if: always() with: path: node_modules/.cache/eslint - key: ${{ runner.os }}-eslint-${{ hashFiles('eslint.config.mjs', 'config/eslint/**', 'lint-deps-lock.json') }}-${{ github.sha }} + key: ${{ runner.os }}-eslint-${{ hashFiles('eslint.config.mjs', 'config/eslint/**', 'eslint-plugin-local-rules/**', 'src/libs/SnapshotUtils.ts', 'src/CONST/runtimeDefaults.ts', 'lint-deps-lock.json') }}-${{ github.sha }} # If lint tightened the seatbelt baseline (i.e. fewer baselined errors than last time), # commit the updated TSV back to `main` as OSBotify. Only runs under `workflow_call` diff --git a/.github/workflows/onyxGetReviewers.yml b/.github/workflows/onyxGetReviewers.yml new file mode 100644 index 000000000000..89477a173728 --- /dev/null +++ b/.github/workflows/onyxGetReviewers.yml @@ -0,0 +1,89 @@ +name: Onyx.get reviewers + +# Requests a review from the Onyx performance reviewers whenever a PR adds a new +# `Onyx.get` or `Onyx.multiGet` call. CODEOWNERS can't express this because it triggers on file +# paths, not on the content of a diff. + +on: + pull_request_target: + types: [opened, synchronize, ready_for_review] + +permissions: + contents: read + pull-requests: write + issues: write + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + requestReviewers: + name: Request reviewers for new Onyx.get and Onyx.multiGet calls + if: ${{ github.event.pull_request.draft == false }} + runs-on: ubuntu-latest + steps: + - name: Request reviewers and leave a comment + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v7 + with: + script: | + const REVIEWERS = ['tgolen', 'mountiny', 'chuckdries', 'luacmartins']; + const pr = context.payload.pull_request; + const {owner, repo} = context.repo; + + // Trigger only on a net-new production call. Only code under `src/` counts: docs, lint rules and + // scripts mention `Onyx.get(` in prose and messages. Jest mocks are excluded. A call matches + // `Onyx.get(` or `Onyx.multiGet(`, not `Onyx.getAllKeys(` or the dev-console `window.Onyx.get(`. + // Added and removed call lines are compared per file, so editing a call in place nets to zero and + // a removal in one file can't mask a new call in another. Lines that start with a comment don't + // count on either side. + const CALL = /(? { + if (!file.patch || !SOURCE_FILE.test(file.filename) || MOCK_DIRECTORY.test(file.filename)) { + return false; + } + let added = 0; + let removed = 0; + for (const line of file.patch.split('\n')) { + const code = line.slice(1); + if (COMMENT_LINE.test(code) || !CALL.test(code)) { + continue; + } + if (line.startsWith('+') && !line.startsWith('+++')) { + added++; + } else if (line.startsWith('-') && !line.startsWith('---')) { + removed++; + } + } + return added > removed; + }); + + if (!addsNewCall) { + return; + } + + // Don't request the PR author on their own PR, anyone already requested, or anyone who + // already reviewed, so re-runs on `synchronize` never create duplicate requests. + const alreadyRequested = (pr.requested_reviewers ?? []).map((user) => user.login); + const reviews = await github.paginate(github.rest.pulls.listReviews, {owner, repo, pull_number: pr.number}); + const alreadyReviewed = reviews.map((review) => review.user?.login).filter(Boolean); + const skip = new Set([pr.user.login, ...alreadyRequested, ...alreadyReviewed]); + const reviewersToRequest = REVIEWERS.filter((reviewer) => !skip.has(reviewer)); + + if (reviewersToRequest.length === 0) { + return; + } + + await github.rest.pulls.requestReviewers({owner, repo, pull_number: pr.number, reviewers: reviewersToRequest}); + + const mentions = REVIEWERS.map((reviewer) => `@${reviewer}`).join(', '); + await github.rest.issues.createComment({ + owner, + repo, + issue_number: pr.number, + body: `This PR adds a new \`Onyx.get\` or \`Onyx.multiGet\` call, so I've requested a review from the Onyx performance reviewers (${mentions}). A review from any one of them is enough. Please check the call against the \`Onyx.get()\` rules in [ONYX-DATA-MANAGEMENT](https://github.com/Expensify/App/blob/main/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md) and [ONYX-1](https://github.com/Expensify/App/blob/main/.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md).`, + }); diff --git a/CLAUDE.md b/CLAUDE.md index 1340a6c1dfde..746f1398332a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,9 +33,9 @@ Onyx (`react-native-onyx`) is our custom offline-first key-value store; keys are defined in `src/ONYXKEYS.ts`. -### Reading Onyx data (`useOnyx` vs `Onyx.connectWithoutView`) +### Reading Onyx data (`useOnyx`, `Onyx.connectWithoutView`, `Onyx.get`) -Prefer pure functions taking data as params over direct Onyx reads — callers read and pass data in. When a read is needed, use **`useOnyx`** (`@hooks/useOnyx`) by default; reach for **`Onyx.connectWithoutView`** only for non-render logic (actions/libraries) that genuinely can't use `useOnyx`. Don't copy-paste `Onyx.connectWithoutView` usages — justify each with a comment. Using it in components for perf needs `@frontend-performance` Slack approval linked in PR description. See [Onyx Data Management](contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md#reading-onyx-data-useonyx-vs-onyxconnectwithoutview). +Prefer pure functions taking data as params over direct Onyx reads: callers read and pass data in. When a read is needed, use **`useOnyx`** (`@hooks/useOnyx`) by default; reach for **`Onyx.connectWithoutView`** only for non-render logic (actions/libraries) that genuinely can't use `useOnyx`. Don't copy-paste `Onyx.connectWithoutView` usages. Justify each with a comment. Using it in components for perf needs `@frontend-performance` Slack approval linked in PR description. `Onyx.get` and `Onyx.multiGet` are one-shot reads for event handlers in components, pages and hooks. See [Onyx Data Management](contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md#reading-onyx-data-useonyx-onyxconnectwithoutview-onyxget-and-onyxmultiget). ## Related Repositories diff --git a/config/eslint/eslint.config.mjs b/config/eslint/eslint.config.mjs index 1194e9100074..f71f861de418 100644 --- a/config/eslint/eslint.config.mjs +++ b/config/eslint/eslint.config.mjs @@ -315,6 +315,8 @@ const config = defineConfig([ 'rulesdir/no-layout-spacing-conditional': 'error', 'rulesdir/no-direct-personal-details-list': 'error', 'rulesdir/require-locale-for-localized-date-format': 'error', + 'rulesdir/no-unsafe-onyx-read': 'error', + 'rulesdir/no-onyx-get-snapshot-key': 'error', 'rulesdir/prefer-narrow-hook-dependencies': [ 'error', { diff --git a/contributingGuides/philosophies/DATA-BINDING.md b/contributingGuides/philosophies/DATA-BINDING.md index 81c80871a6bf..4b2d6fb60deb 100644 --- a/contributingGuides/philosophies/DATA-BINDING.md +++ b/contributingGuides/philosophies/DATA-BINDING.md @@ -42,7 +42,7 @@ function getReportName(reportID: string): string { ### - Library files that are not connected or associated to any UI SHOULD use `Onyx.connectWithoutView()` to subscribe to changes in Onyx data Library files are located in `/src/lib` but excluding the actions in `/src/lib/actions` which have their own rule above. -`useOnyx()` is the standard for accessing Onyx data for React components. Non-React library files should always prefer using pure functions that receive all data as parameters. `Onyx.connectWithoutView()` should only be used when a library file is unable to get the data with `useOnyx()` (eg. non-React code, network layer, etc.). Never add a reference to `Onyx.connectWithoutView()` just because nearby code uses it. See [Reading Onyx data: `useOnyx` vs `Onyx.connectWithoutView`](/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md#reading-onyx-data-useonyx-vs-onyxconnectwithoutview) for the full rules. +`useOnyx()` is the standard for accessing Onyx data for React components. Non-React library files should always prefer using pure functions that receive all data as parameters. `Onyx.connectWithoutView()` should only be used when a library file is unable to get the data with `useOnyx()` (eg. non-React code, network layer, etc.). Never add a reference to `Onyx.connectWithoutView()` just because nearby code uses it. See [Reading Onyx data: `useOnyx`, `Onyx.connectWithoutView`, `Onyx.get()` and `Onyx.multiGet()`](/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md#reading-onyx-data-useonyx-onyxconnectwithoutview-onyxget-and-onyxmultiget) for the full rules. ```ts // GOOD ✅ — non-React network layer keeps the auth token in module state diff --git a/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md b/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md index 60c7fb78c89d..79a6944862a4 100644 --- a/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md +++ b/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md @@ -16,6 +16,7 @@ This is how the application manages all the data stored in Onyx. This improves performance and lessens the chance that one action will overwrite the changes made by another action. ### - UI Components MUST NOT call Onyx methods directly and should call an action instead +The only exception is a one-shot `Onyx.get()` or `Onyx.multiGet()` in an event handler of a component, page or hook, under the reading rules below. ### - Data SHOULD be optimistically stored on disk whenever possible without waiting for a server response Example of creating a new optimistic comment: 1. User adds a comment @@ -37,13 +38,15 @@ Different platforms come with varying storage capacities and Onyx has a way to g - Add the key to the `evictableKeys` option in `Onyx.init(options)` - A least recently accessed key will only be deleted when an Onyx operation retries after failing. -## Reading Onyx data: `useOnyx` vs `Onyx.connectWithoutView` -There are only two ways to read Onyx data, and `Onyx.connect` is deprecated: +## Reading Onyx data: `useOnyx`, `Onyx.connectWithoutView`, `Onyx.get()` and `Onyx.multiGet()` +There are four ways to read Onyx data, and `Onyx.connect` is deprecated: 1. **`useOnyx`** (from `@hooks/useOnyx`) — the default for anything a React component renders. 2. **`Onyx.connectWithoutView`** — an imperative subscription for non-render logic, used only when `useOnyx` genuinely does not fit. +3. **`Onyx.get()`**: an asynchronous, one-shot read of the cache that never subscribes, for event handlers in components, pages and hooks. +4. **`Onyx.multiGet()`**: takes an array of keys and resolves each one the way `Onyx.get()` does, in the order given. Every `Onyx.get()` rule below applies to it and to each key it reads. ### - Prefer a pure function over reading Onyx at all -A pure function does not read Onyx itself — it receives the data it needs as parameters, and its caller does the reading (with `useOnyx` or `Onyx.connectWithoutView`) and passes it in. Before adding either subscription, check whether the code can be a pure function instead: it needs no connection, is trivial to test, and cannot cause extra rerenders. Prefer this even when it means passing more arguments. This takes precedence over everything below. +A pure function does not read Onyx itself. It receives the data it needs as parameters, and its caller does the reading (with `useOnyx`, `Onyx.connectWithoutView` or, in an event handler, `Onyx.get()`) and passes it in. Before adding either subscription, check whether the code can be a pure function instead: it needs no connection, is trivial to test, and cannot cause extra rerenders. Prefer this even when it means passing more arguments. This takes precedence over everything below. ### - Components MUST read Onyx with `useOnyx`, never `Onyx.connectWithoutView` Any value used during render belongs in `useOnyx` so the UI updates when the value changes. @@ -60,6 +63,25 @@ Add an inline comment at each new `Onyx.connectWithoutView` call stating why the ### - Using `Onyx.connectWithoutView` in a component for performance REQUIRES @frontend-performance approval In rare cases a component that subscribes to multiple large collections through `useOnyx` suffers a significant performance regression. Reaching for `Onyx.connectWithoutView` to avoid that is an explicit exception, not a self-serve option: it MUST be approved by the `@frontend-performance` team on Slack, and the PR description MUST link to that discussion. +### - `Onyx.get()` is ONLY for event handlers in components, pages and hooks +It reads the cache once and never subscribes, so the value it returns MUST NOT reach rendered output, directly or through state, a ref or a module variable. Use it in event handlers and `useCallback` bodies under `src/components`, `src/pages` and `src/hooks`. Never call it during render, at module scope, or in code an effect runs. + +### - `Onyx.get()` MUST NOT read the Search snapshot keys +`@hooks/useOnyx` redirects the keys in `CONST.SEARCH.SNAPSHOT_ONYX_KEYS` to a Search snapshot inside a `SearchScopeProvider`. `Onyx.get()` always reads the global key. These keys stay on `useOnyx`, and an `Onyx.multiGet()` key list must not contain any of them. Write that list as an array literal of static keys, or as a `const` bound to one. Declare that `const` once in the same file, do not export it, and pass it only to `Onyx.multiGet()`, so lint can check each element. + +Read live data only when the read replaces a subscription that never went through the `useOnyx` redirection, such as a module-level `Onyx.connect()` used only from handlers. In that case, disable `rulesdir/no-onyx-get-snapshot-key` on that line with a reason after `--`. For `Onyx.multiGet()`, disable it on the line of the call, which covers every key in the list. For example: `// eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- replaces a module-level Onyx.connect used only by handlers, never read a snapshot`. Disable one line at a time, always with a reason. Reviewers judge the reason. A read of the Concierge chat always needs the live report, in any Search scope. `Onyx.get()` may read `${ONYXKEYS.COLLECTION.REPORT}${conciergeReportID}` without a disable when `conciergeReportID` is a same-file `const` bound to a read of `ONYXKEYS.CONCIERGE_REPORT_ID`. + +### - Reads MUST come before a merge, mergeCollection, setCollection, update or clear in the same tick, or after it is awaited +`Onyx.get()` reads the cache when it is called. `Onyx.set()` and `Onyx.multiSet()` update the cache before they return, but `Onyx.merge()`, `Onyx.mergeCollection()`, `Onyx.setCollection()`, `Onyx.clear()` and every `Onyx.update()` operation, including SET, land after at least one microtask. A read queued behind one of those returns the old value. A derived key (`ONYXKEYS.DERIVED.*`) lags its sources, so read it only before writing them. + +### - A subscription that triggers work MUST stay on `useOnyx` +If the value re-runs an effect, directly or through a callback in a dependency array, a one-shot read stops that effect from re-running. + +### - Reapply the `selector` and never mutate the result +`Onyx.get()` returns the stored value without the `selector` projection that `useOnyx` applies. A read returns the cached object itself, for a single key and for every member of a collection, so writing to it changes the cache without telling subscribers. Calling `Onyx.set()` with the mutated object does not fix this. `set` compares it with the cached object, which is the same object, so it can skip the write and the notification. Build a new object instead. + +`rulesdir/no-unsafe-onyx-read` enforces where a read may run, that it sits inside a component or hook in those directories, and that `Onyx.get` and `Onyx.multiGet` are called directly. It cannot be disabled inline in any linted file outside `tests/`. `rulesdir/no-onyx-get-snapshot-key` enforces which keys `Onyx.get()` may read. It can be disabled only with a reason after `--`. Reviewers check the rest with [ONYX-1](../../.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md), which has examples. + ## Onyx Derived Values Derived values are special Onyx keys which contain values derived from other Onyx values. These are available as a performance optimization, so that if the result of a common computation of Onyx values is needed in many places across the app, the computation can be done only as needed in a centralized location, and then shared across the app. Once created, Onyx derived values are stored and consumed just like any other Onyx value. diff --git a/eslint-plugin-local-rules/no-onyx-get-snapshot-key.js b/eslint-plugin-local-rules/no-onyx-get-snapshot-key.js new file mode 100644 index 000000000000..0b61af840c7e --- /dev/null +++ b/eslint-plugin-local-rules/no-onyx-get-snapshot-key.js @@ -0,0 +1,328 @@ +import {createRequire} from 'node:module'; +import path from 'path'; + +/* eslint-disable import/extensions */ +// Node's ESM loader does not add file extensions, so the helper import needs `.js`, which import/extensions reports +import { + createOnyxReadTracker, + getConstInitializer, + getKeyListElements, + getOnyxKeyPath, + getSingleVariableDefinition, + getStaticName, + MULTI_READ_METHOD, + ONYX_MODULE, + ONYXKEYS_ROOT, + READ_METHOD, + REPO_ROOT, + resolveVariable, + SNAPSHOT_KEYS_SOURCE, + unwrapTypeOnlyExpressions, +} from './utils/onyxReadUtils.js'; +/* eslint-enable import/extensions */ + +const name = 'no-onyx-get-snapshot-key'; + +// A Concierge chat read wants the live report in any Search scope, so the rule allows a report key it can prove is built from this ID +const CONCIERGE_REPORT_ID_KEY_PATH = 'CONCIERGE_REPORT_ID'; + +const REPORT_COLLECTION_KEY_PATH = 'COLLECTION.REPORT'; + +const USE_ONYX_HOOK_SOURCE = /(^|\/)useOnyx$/; + +const USE_ONYX_EXPORT_NAME = 'useOnyx'; + +const SELECTOR_OPTION_NAME = 'selector'; + +if (!REPO_ROOT) { + throw new Error( + `${name} could not find ${SNAPSHOT_KEYS_SOURCE} two directories above eslint-plugin-local-rules/utils/. Without it the rule cannot tell which keys are Search snapshot keys, so it stops instead of letting them through.`, + ); +} + +// ESLint's --cache keeps a file's result until that file or the ESLint config changes. It does not track the modules loaded +// below, so after an edit to SnapshotUtils.ts, ONYXKEYS.ts or runtimeDefaults.ts a cached pass can be stale. Lint with --no-cache then. +const requireFromRepo = createRequire(path.join(REPO_ROOT, 'package.json')); + +let snapshotKeyResolver = null; + +/** Loads the key check shared with useOnyx on first use. SnapshotUtils.ts, ONYXKEYS.ts and runtimeDefaults.ts take tens of ms to load, so a lint worker pays it only once it meets an Onyx.get or Onyx.multiGet. */ +function getSnapshotKeyResolver() { + if (snapshotKeyResolver) { + return snapshotKeyResolver; + } + + const snapshotKeys = requireFromRepo(`./${SNAPSHOT_KEYS_SOURCE}`).default?.SEARCH_SNAPSHOT_ONYX_KEYS; + + if (!Array.isArray(snapshotKeys) || snapshotKeys.length === 0) { + throw new Error( + `${name} found no SEARCH_SNAPSHOT_ONYX_KEYS entries in the default export of ${SNAPSHOT_KEYS_SOURCE}. Without them the rule cannot tell which keys are Search snapshot keys, so it stops instead of letting them through.`, + ); + } + + snapshotKeyResolver = { + isSnapshotCompatibleKey: requireFromRepo('./src/libs/SnapshotUtils.ts').default, + onyxKeys: requireFromRepo('./src/ONYXKEYS.ts').default, + snapshotKeys, + }; + + return snapshotKeyResolver; +} + +function getOnyxKeyValue(keyPath) { + const value = keyPath.split('.').reduce((node, segment) => node?.[segment], getSnapshotKeyResolver().onyxKeys); + + return typeof value === 'string' ? value : null; +} + +const DISABLE_GUIDANCE = + 'Live data is correct only when the read replaces a subscription that never went through the useOnyx() redirection, such as a module-level Onyx.connect() used only by handlers. ' + + 'In that case disable this rule on the line with a reason after `--`, for example: `// eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- replaces a module-level Onyx.connect used only by handlers, never read a snapshot`.'; + +const meta = { + type: 'problem', + docs: { + description: 'Disallow Onyx.get and Onyx.multiGet on Search snapshot keys and on keys the rule cannot resolve. Disable it only with a reason after `--`.', + recommended: 'error', + }, + schema: [], + messages: { + noRestrictedOnyxKey: [ + 'Do not read {{keyPath}} with Onyx.get() or Onyx.multiGet(). Inside a SearchScopeProvider subtree, src/hooks/useOnyx.ts redirects this key to snapshot_, so this read returns live data where the component showed the snapshot.', + 'Take the value as a parameter from the component, which knows whether it is inside a Search scope, or keep the useOnyx subscription. A Concierge chat read is allowed, because it always wants the live report: build the report key from a const in this file bound to a read of ONYXKEYS.CONCIERGE_REPORT_ID.', + DISABLE_GUIDANCE, + ].join('\n\n'), + noUnresolvableOnyxKey: [ + 'Do not read Onyx with `{{keyText}}`, a key this rule cannot resolve. The rule cannot check a key built at runtime, so a Search snapshot key could reach this read without any error.', + 'Write the key as an ONYXKEYS access, such as ONYXKEYS.SESSION, or as a template literal that starts with an ONYXKEYS collection prefix. For Onyx.multiGet(), pass an array literal, or a const bound to one, with every element written that way. If the key cannot be static, keep the useOnyx subscription or take the value as a parameter.', + DISABLE_GUIDANCE, + ].join('\n\n'), + }, +}; + +// `(await Onyx.get(key)) as string` and `await (Onyx.get(key) as Promise)` both resolve to the call +function unwrapAwaitAndTypeOnlyExpressions(node) { + let current = unwrapTypeOnlyExpressions(node); + + while (current?.type === 'AwaitExpression') { + current = unwrapTypeOnlyExpressions(current.argument); + } + + return current; +} + +/** The index of `variableName` in `arrayPattern`, or null. An element with a default counts only when the default is a literal, so a missing ID cannot fall back to another report's ID. */ +function getBoundElementIndex(arrayPattern, variableName) { + return arrayPattern.elements.findIndex((element) => { + if (element?.type === 'AssignmentPattern') { + return element.left.type === 'Identifier' && element.left.name === variableName && element.right.type === 'Literal'; + } + + return element?.type === 'Identifier' && element.name === variableName; + }); +} + +function hasSelectorOption(call) { + const options = call.arguments.at(1); + + if (!options) { + return false; + } + + // Anything but a plain object literal could carry a selector, which replaces the tuple value with something else + return ( + options.type !== 'ObjectExpression' || options.properties.some((property) => property.type !== 'Property' || getStaticName(property.key, property.computed) === SELECTOR_OPTION_NAME) + ); +} + +function create(context) { + const sourceCode = context.sourceCode ?? context.getSourceCode(); + const {getCalledReadMethod, getImportSpecifier} = createOnyxReadTracker(sourceCode); + + function isMultiReadCall(call) { + return getCalledReadMethod(call.callee) === MULTI_READ_METHOD; + } + + /** True for a call of `import useOnyx from '@hooks/useOnyx'` or `import {useOnyx as x} from 'react-native-onyx'`. */ + function isUseOnyxCall(call) { + if (call.callee.type !== 'Identifier') { + return false; + } + + const hookModuleSpecifier = getImportSpecifier(call.callee, (source) => USE_ONYX_HOOK_SOURCE.test(source)); + + if (hookModuleSpecifier) { + return hookModuleSpecifier.type === 'ImportDefaultSpecifier'; + } + + const onyxSpecifier = getImportSpecifier(call.callee, (source) => source === ONYX_MODULE); + + return onyxSpecifier?.type === 'ImportSpecifier' && getStaticName(onyxSpecifier.imported, false) === USE_ONYX_EXPORT_NAME; + } + + /** The ONYXKEYS member path `node` reads, or null. Unlike getOnyxKeyPath it rejects a template literal, whose leading ONYXKEYS prefix would otherwise count as the whole key. */ + function getMemberKeyPath(node, seen = new Set()) { + const current = unwrapTypeOnlyExpressions(node); + + if (current?.type === 'Identifier' && current.name !== ONYXKEYS_ROOT) { + if (seen.has(current)) { + return null; + } + + seen.add(current); + const initializer = getConstInitializer(current, sourceCode); + + return initializer ? getMemberKeyPath(initializer, seen) : null; + } + + return current?.type === 'MemberExpression' ? getOnyxKeyPath(current, sourceCode) : null; + } + + /** True for `Onyx.get(ONYXKEYS.CONCIERGE_REPORT_ID)`, also through a const alias of Onyx.get. A `let` alias does not count, since it could be reassigned before the read. */ + function readsConciergeReportID(call) { + return ( + call?.type === 'CallExpression' && getCalledReadMethod(call.callee, {constOnly: true}) === READ_METHOD && getMemberKeyPath(call.arguments.at(0)) === CONCIERGE_REPORT_ID_KEY_PATH + ); + } + + function tupleElementIsConciergeReportID(call, elementIndex) { + if (call?.type !== 'CallExpression') { + return false; + } + + if (isUseOnyxCall(call)) { + return elementIndex === 0 && !hasSelectorOption(call) && getMemberKeyPath(call.arguments.at(0)) === CONCIERGE_REPORT_ID_KEY_PATH; + } + + if (getCalledReadMethod(call.callee, {constOnly: true}) !== MULTI_READ_METHOD) { + return false; + } + + const keyElements = getKeyListElements(call.arguments.at(0), sourceCode, isMultiReadCall)?.slice(0, elementIndex + 1); + + // A spread before the slot shifts every later index at runtime, so the slot cannot be matched to a key + if (keyElements?.length !== elementIndex + 1 || keyElements.some((element) => !element || element.type === 'SpreadElement')) { + return false; + } + + return getMemberKeyPath(keyElements.at(elementIndex)) === CONCIERGE_REPORT_ID_KEY_PATH; + } + + /** True when `node` is a const declared in this file and bound to a read of ONYXKEYS.CONCIERGE_REPORT_ID. */ + function isConciergeReportIDExpression(node) { + const current = unwrapTypeOnlyExpressions(node); + + if (current?.type !== 'Identifier') { + return false; + } + + const definition = getSingleVariableDefinition(resolveVariable(current, sourceCode), true); + + if (!definition) { + return false; + } + + const {id, init} = definition.node; + const call = unwrapAwaitAndTypeOnlyExpressions(init); + + if (id.type === 'Identifier') { + return readsConciergeReportID(call); + } + + if (id.type !== 'ArrayPattern') { + return false; + } + + const elementIndex = getBoundElementIndex(id, current.name); + + return elementIndex !== -1 && tupleElementIsConciergeReportID(call, elementIndex); + } + + /** True for `${ONYXKEYS.COLLECTION.REPORT}${conciergeReportID}`, written inline or through a const. */ + function isConciergeChatKey(node, seen = new Set()) { + const current = unwrapTypeOnlyExpressions(node); + + if (current?.type === 'Identifier') { + if (seen.has(current)) { + return false; + } + + seen.add(current); + const initializer = getConstInitializer(current, sourceCode); + + return !!initializer && isConciergeChatKey(initializer, seen); + } + + // cspell:disable-next-line -- quasis is the ESTree name for the static chunks of a template literal + if (current?.type !== 'TemplateLiteral' || current.expressions.length !== 2 || current.quasis.some((quasi) => quasi.value.cooked !== '')) { + return false; + } + + return getMemberKeyPath(current.expressions.at(0)) === REPORT_COLLECTION_KEY_PATH && isConciergeReportIDExpression(current.expressions.at(1)); + } + + /** Null when the key is allowed, `{keyPath}` for a Search snapshot key, `{keyPath: null, keyNode}` for an unresolvable key. */ + function findRestrictedKey(keyArgument) { + const keyPath = getOnyxKeyPath(keyArgument, sourceCode); + const key = keyPath ? getOnyxKeyValue(keyPath) : null; + + if (!key) { + return {keyPath: null, keyNode: keyArgument}; + } + + const {isSnapshotCompatibleKey, snapshotKeys} = getSnapshotKeyResolver(); + + if (!isSnapshotCompatibleKey(key, snapshotKeys) || isConciergeChatKey(keyArgument)) { + return null; + } + + return {keyPath}; + } + + function findRestrictedKeys(readMethod, call) { + const keyArgument = call.arguments.at(0); + + if (readMethod !== MULTI_READ_METHOD) { + const finding = findRestrictedKey(keyArgument); + + return finding ? [{node: call, ...finding}] : []; + } + + const elements = keyArgument?.type === 'SpreadElement' ? null : getKeyListElements(keyArgument, sourceCode, isMultiReadCall); + + if (!elements) { + return [{node: call, keyPath: null, keyNode: keyArgument}]; + } + + // Every finding is reported on the call, so one directive above a multi-line call covers all of its keys + return elements.flatMap((element) => { + if (!element || element.type === 'SpreadElement') { + return [{node: call, keyPath: null, keyNode: element}]; + } + + const finding = findRestrictedKey(element); + + return finding ? [{node: call, ...finding}] : []; + }); + } + + return { + CallExpression(node) { + const readMethod = getCalledReadMethod(node.callee); + + if (!readMethod) { + return; + } + + for (const finding of findRestrictedKeys(readMethod, node)) { + context.report( + finding.keyPath + ? {node: finding.node, messageId: 'noRestrictedOnyxKey', data: {keyPath: `${ONYXKEYS_ROOT}.${finding.keyPath}`}} + : {node: finding.node, messageId: 'noUnresolvableOnyxKey', data: {keyText: finding.keyNode ? sourceCode.getText(finding.keyNode) : 'no key'}}, + ); + } + }, + }; +} + +export {name, meta, create}; diff --git a/eslint-plugin-local-rules/no-unsafe-onyx-read.js b/eslint-plugin-local-rules/no-unsafe-onyx-read.js new file mode 100644 index 000000000000..fa496722f67b --- /dev/null +++ b/eslint-plugin-local-rules/no-unsafe-onyx-read.js @@ -0,0 +1,851 @@ +// Node's ESM loader does not add file extensions, so the helper import needs `.js`, which import/extensions reports +/* eslint-disable import/extensions */ +import { + createOnyxReadTracker, + getCalleeName, + getDeclaredVariable, + getRepoRelativePath, + getStaticName, + matchesCalleeName, + ONYX_MODULE, + TYPE_ONLY_EXPRESSIONS, + unwrapTypeOnlyExpressions, +} from './utils/onyxReadUtils.js'; +/* eslint-enable import/extensions */ + +const name = 'no-unsafe-onyx-read'; + +const TESTS_DIRECTORY = 'tests/'; + +const READ_ALLOWED_DIRECTORIES = ['src/components/', 'src/pages/', 'src/hooks/', TESTS_DIRECTORY]; + +// Test suites, Jest setup and the root __mocks__/ directory may use the Onyx object as a value, as the `...Onyx` in +// __mocks__/react-native-onyx.ts does. A nested __mocks__/ directory does not count. +const TEST_INFRASTRUCTURE_DIRECTORIES = [TESTS_DIRECTORY, '__mocks__/', 'jest/']; + +const READ_ALLOWED_FILES = new Set([]); + +// Hooks that run their callback after render, mapped to the argument index of that callback +const EFFECT_HOOK_CALLBACK_INDICES = new Map([ + ['useEffect', 0], + ['useLayoutEffect', 0], + ['useInsertionEffect', 0], + ['useFocusEffect', 0], + ['useImperativeHandle', 1], +]); + +const CALLBACK_HOOK_NAMES = new Set(['useCallback', 'useEffectEvent']); + +const EFFECT_CONTINUATION_NAMES = new Set([ + 'then', + 'catch', + 'finally', + 'setTimeout', + 'setInterval', + 'requestAnimationFrame', + 'requestIdleCallback', + 'queueMicrotask', + 'runAfterInteractions', + 'runAfterTransitions', + 'startTransition', + 'setNavigationActionToMicrotaskQueue', +]); + +const SYNCHRONOUS_CALLBACK_METHODS = new Set([ + 'from', + 'map', + 'filter', + 'reduce', + 'reduceRight', + 'forEach', + 'find', + 'findIndex', + 'findLast', + 'findLastIndex', + 'flatMap', + 'some', + 'every', + 'sort', +]); + +const RENDER_TIME_HOOK_ARGUMENTS = new Map([ + ['useMemo', new Set([0])], + ['useState', new Set([0])], + ['useReducer', new Set([2])], + ['useSyncExternalStore', new Set([1, 2])], +]); + +const RENDER_TIME_OPTION_NAMES = new Set(['selector']); + +const DEV_CONSOLE_GLOBALS = new Set(['window', 'globalThis']); + +// The one property react-native-onyx.d.ts declares on Window +const DEV_CONSOLE_PROPERTY = 'Onyx'; + +const COMPONENT_WRAPPER_NAMES = new Set(['memo', 'forwardRef']); + +const SYNCHRONOUS_EXECUTOR_NAMES = new Set(['Promise']); + +const RENDER = 'render'; +const DEFERRED = 'deferred'; +const SYNCHRONOUS = 'synchronous'; + +const MODULE_SCOPE = 'moduleScope'; +const EVENT = 'event'; +const EFFECT = 'effect'; + +const meta = { + type: 'problem', + docs: { + description: + 'Disallow Onyx.get and Onyx.multiGet outside components, pages, hooks and tests, outside a component or hook body in those source directories, during render, inside effects or at module scope, and disallow using the Onyx object as a value instead of through its members. no-onyx-get-snapshot-key checks which keys they read.', + recommended: 'error', + }, + schema: [], + messages: { + noOnyxReadInRender: + 'Do not read Onyx during render. Onyx.get() and Onyx.multiGet() do not subscribe, so the component keeps showing the value it read after the key changes.\n\n' + + 'Use useOnyx() for anything the component renders, and keep Onyx.get() and Onyx.multiGet() for event handlers that render does not call.', + noOnyxReadAtModuleScope: + 'Do not read Onyx at module scope. The module body runs once at import, so a value read there never updates when the key changes.\n\n' + + 'Move the read into the function that needs the value, so it reads the current value when that function runs. If the module must track a key, subscribe with Onyx.connectWithoutView().', + noOnyxReadOutsideAllowedPath: + 'Onyx.get() and Onyx.multiGet() are only allowed in src/components, src/pages, src/hooks and tests. Outside them this rule cannot tell whether a read runs during render, in an effect or in an event handler.\n\n' + + 'Take the value as a parameter or keep the Onyx.connectWithoutView() subscription. Reviewers accept a new READ_ALLOWED_FILES entry only in a PR that removes an Onyx.connectWithoutView() from that file.', + noOnyxReadInEffect: + 'Do not read Onyx inside an effect or in a function an effect calls. If the value is an effect dependency, its useOnyx subscription is what re-runs the effect, and a one-shot read stops that.\n\n' + + 'Keep the useOnyx subscription. This rule cannot yet tell whether the value is a dependency, so it reports every read in an effect.', + noOnyxReadOutsideComponentOrHook: + 'Do not call Onyx.get() or Onyx.multiGet() in a module-level helper. Files in src/libs can import the helper, and these reads are banned there.\n\n' + + 'Read inside the component or hook that needs the value, or in a handler defined inside it, and pass the value to the helper as a parameter.', + noOnyxReadByReference: + 'Do not use Onyx.get or Onyx.multiGet other than as a direct call, as in Onyx.get.call, Onyx.get.bind, keys.map(Onyx.get), const read = Onyx.get, const {get} = Onyx or Onyx[name]. Such a reference hides the key from no-onyx-get-snapshot-key and the call site from this rule.\n\n' + + 'Call Onyx.get() or Onyx.multiGet() directly.', + noOnyxObjectAsValue: + 'Do not pass, spread, alias, return or export the Onyx object. Reads made through such a value can escape this rule and the key check of no-onyx-get-snapshot-key.\n\n' + + 'Use Onyx through its members, such as Onyx.get() or Onyx.merge(). The one allowed value use is window.Onyx = Onyx or globalThis.Onyx = Onyx for the dev console.', + }, +}; + +function isFunctionNode(node) { + return node.type === 'FunctionDeclaration' || node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression'; +} + +function isHookName(functionName) { + return /^use[A-Z0-9]/.test(functionName); +} + +function isComponentName(functionName) { + return /^[A-Z]/.test(functionName); +} + +function isComponentOrHookName(functionName) { + return !!functionName && (isHookName(functionName) || isComponentName(functionName)); +} + +/** The name a function is declared or assigned under, or null. An object key such as `APPROVE` or `Enter` does not count, because it does not name a component or hook. */ +function getFunctionName(functionNode, parent) { + if (functionNode.id?.type === 'Identifier') { + return functionNode.id.name; + } + + if (parent?.type === 'VariableDeclarator' && parent.id.type === 'Identifier') { + return parent.id.name; + } + + return null; +} + +/** True for JSX written directly, or behind `cond ? : null`, `cond && ` or a TS cast. */ +function isJSXExpression(node) { + if (!node) { + return false; + } + + if (node.type === 'JSXElement' || node.type === 'JSXFragment') { + return true; + } + + if (node.type === 'ConditionalExpression') { + return isJSXExpression(node.consequent) || isJSXExpression(node.alternate); + } + + if (node.type === 'LogicalExpression') { + return isJSXExpression(node.left) || isJSXExpression(node.right); + } + + return TYPE_ONLY_EXPRESSIONS.has(node.type) && isJSXExpression(node.expression); +} + +/** True when the function body is JSX or has a top-level `return` of JSX. A `return` nested in a block is not checked. */ +function returnsJSX(functionNode) { + const body = functionNode.body; + + if (!body) { + return false; + } + + if (body.type !== 'BlockStatement') { + return isJSXExpression(body); + } + + return body.body.some((statement) => statement.type === 'ReturnStatement' && isJSXExpression(statement.argument)); +} + +function getRenderTimeArgumentIndices(callee) { + const calleeName = getCalleeName(callee); + + return calleeName ? (RENDER_TIME_HOOK_ARGUMENTS.get(calleeName) ?? null) : null; +} + +function getOutermostTypeOnlyWrapper(node) { + let current = node; + + while (current?.parent && TYPE_ONLY_EXPRESSIONS.has(current.parent.type)) { + current = current.parent; + } + + return current; +} + +function isHookArgument(node, call) { + if (call?.type !== 'CallExpression' || !call.arguments.includes(node)) { + return false; + } + + const calleeName = getCalleeName(call.callee); + + return !!calleeName && isHookName(calleeName); +} + +/** True when `property` sits in the options object of a hook call, inline as in `useOnyx(key, {selector})` or through `const options = {selector}; useOnyx(key, options)`, with TS casts on either side. */ +function isHookOption(property, sourceCode) { + const holder = getOutermostTypeOnlyWrapper(property.parent); + + if (isHookArgument(holder, holder?.parent)) { + return true; + } + + const declarator = holder?.parent; + + if (declarator?.type !== 'VariableDeclarator' || declarator.init !== holder || declarator.id.type !== 'Identifier') { + return false; + } + + const variable = getDeclaredVariable(declarator, declarator.id.name, sourceCode); + + return ( + !!variable && + variable.references.some((reference) => { + const argument = getOutermostTypeOnlyWrapper(reference.identifier); + + return isHookArgument(argument, argument.parent); + }) + ); +} + +function isRenderTimeUsage(identifier, sourceCode) { + const parent = identifier.parent; + + if (parent?.type === 'Property' && parent.value === identifier && RENDER_TIME_OPTION_NAMES.has(getStaticName(parent.key, parent.computed)) && isHookOption(parent, sourceCode)) { + return true; + } + + if (parent?.type !== 'CallExpression' || !parent.arguments.includes(identifier)) { + return false; + } + + return matchesCalleeName(parent.callee, COMPONENT_WRAPPER_NAMES) || !!getRenderTimeArgumentIndices(parent.callee)?.has(parent.arguments.indexOf(identifier)); +} + +/** How the function can be referred to: `{declaration, name}` for a named function or variable, `{definition, classBody, key}` for a class member, `{definition, holder, key}` for a property of a `const` or `let` object, or null. */ +function getFunctionBinding(functionNode, parent) { + if (functionNode.type === 'FunctionDeclaration' && functionNode.id?.type === 'Identifier') { + return {declaration: functionNode, name: functionNode.id.name}; + } + + if (parent?.type === 'VariableDeclarator' && parent.init === functionNode && parent.id.type === 'Identifier') { + return {declaration: parent, name: parent.id.name}; + } + + const key = parent && parent.value === functionNode ? getStaticName(parent.key, parent.computed) : null; + + if (!key) { + return null; + } + + if (parent.type === 'PropertyDefinition' || parent.type === 'MethodDefinition') { + return {definition: parent, classBody: parent.parent, key}; + } + + const holder = parent.type === 'Property' ? parent.parent.parent : null; + + if (holder?.type === 'VariableDeclarator' && holder.init === parent.parent && holder.id.type === 'Identifier' && (holder.parent.kind === 'const' || holder.parent.kind === 'let')) { + return {definition: parent, holder: {declaration: holder, name: holder.id.name}, key}; + } + + return null; +} + +function getReadIdentifiers(declaration, boundName, sourceCode) { + const variable = getDeclaredVariable(declaration, boundName, sourceCode); + + return variable ? variable.references.filter((reference) => reference.isRead()).map((reference) => reference.identifier) : []; +} + +function isNamedMember(node, key) { + return node?.type === 'MemberExpression' && getStaticName(node.property, node.computed) === key; +} + +/** Collects the `this.key` uses inside `classBody` into `uses`. Skips nested non-arrow functions and classes, which rebind `this`, but still enters the methods of `classBody`. */ +function collectThisMemberUses(node, classBody, key, sourceCode, uses) { + if (isNamedMember(node, key) && node.object.type === 'ThisExpression') { + uses.push(node); + } + + const rebindsThis = (node.type === 'FunctionExpression' || node.type === 'FunctionDeclaration') && node.parent?.parent !== classBody; + + if (rebindsThis || (node.type === 'ClassBody' && node !== classBody)) { + return; + } + + for (const childKey of sourceCode.visitorKeys[node.type] ?? []) { + for (const child of [node[childKey]].flat()) { + if (child) { + collectThisMemberUses(child, classBody, key, sourceCode, uses); + } + } + } +} + +/** Every place the bound function is referred to: a read of its variable, `holder.key`, or `this.key` inside its class. */ +function getBindingUses(binding, sourceCode, seen) { + const seenKey = binding.definition ?? getDeclaredVariable(binding.declaration, binding.name, sourceCode); + + if (!seenKey || seen.has(seenKey)) { + return []; + } + + seen.add(seenKey); + + if (binding.classBody) { + const uses = []; + collectThisMemberUses(binding.classBody, binding.classBody, binding.key, sourceCode, uses); + return uses; + } + + if (binding.holder) { + return getReadIdentifiers(binding.holder.declaration, binding.holder.name, sourceCode) + .map((identifier) => identifier.parent) + .filter((member) => isNamedMember(member, binding.key)); + } + + return getReadIdentifiers(binding.declaration, binding.name, sourceCode); +} + +/** The `run` binding of `const run = useCallback(load, [])` or `const run = useEffectEvent(load)`, which names the same function as `load`, or null. */ +function getCallbackHookAlias(node) { + const call = node.parent; + const declarator = call?.parent; + + if (call?.type !== 'CallExpression' || call.arguments.at(0) !== node || !matchesCalleeName(call.callee, CALLBACK_HOOK_NAMES)) { + return null; + } + + return declarator?.type === 'VariableDeclarator' && declarator.init === call && declarator.id.type === 'Identifier' ? {declaration: declarator, name: declarator.id.name} : null; +} + +/** The binding of another name for the same function, from `const run = load` or a callback hook wrapping `load`, or null. */ +function getAliasBinding(use) { + const parent = use.parent; + + if (parent?.type === 'VariableDeclarator' && parent.init === use && parent.id.type === 'Identifier') { + return {declaration: parent, name: parent.id.name}; + } + + return getCallbackHookAlias(use); +} + +/** + * RENDER or MODULE_SCOPE when some use of the bound function runs it there, otherwise null. A use counts when it is a + * render-time slot, or a call, synchronous array callback or Promise executor that sits in a render body or at module + * scope, or an alias used in one of those ways. `classifyCall` runs classifyPosition on a call site; it is passed in + * because classifyPosition also calls this function. + */ +function getEscalatedPosition(binding, sourceCode, seen, classifyCall) { + for (const use of getBindingUses(binding, sourceCode, seen)) { + if (isRenderTimeUsage(use, sourceCode)) { + return RENDER; + } + + const parent = use.parent; + + // `const value = helper();` written in a render body or at module scope runs the helper right there + if (parent?.type === 'CallExpression' && parent.callee === use) { + const position = classifyCall(parent); + + if (position === RENDER || position === MODULE_SCOPE) { + return position; + } + + continue; + } + + // `ids.map(helper)` or `new Promise(helper)` runs the helper where that call runs; a timer or `.then` defers it + const isSynchronousCallback = + parent?.type === 'CallExpression' && + parent.arguments.includes(use) && + parent.callee.type === 'MemberExpression' && + matchesCalleeName(parent.callee, SYNCHRONOUS_CALLBACK_METHODS); + const isExecutor = parent?.type === 'NewExpression' && parent.arguments.at(0) === use && matchesCalleeName(parent.callee, SYNCHRONOUS_EXECUTOR_NAMES); + + if (isSynchronousCallback || isExecutor) { + const position = classifyCall(parent); + + if (position === RENDER || position === MODULE_SCOPE) { + return position; + } + + continue; + } + + const alias = getAliasBinding(use); + const position = alias ? getEscalatedPosition(alias, sourceCode, seen, classifyCall) : null; + + if (position) { + return position; + } + } + + return null; +} + +function isEffectHookCallbackArgument(call, argument) { + const calleeName = getCalleeName(call.callee); + + return !!calleeName && EFFECT_HOOK_CALLBACK_INDICES.get(calleeName) === call.arguments.indexOf(argument); +} + +/** True for an effect hook callback, passed directly as in `useEffect(fn)` or through `useEffect(useCallback(fn, []))`. */ +function isEffectCallback(functionNode, parent, grandparent) { + if (parent?.type !== 'CallExpression' || !parent.arguments.includes(functionNode)) { + return false; + } + + if (isEffectHookCallbackArgument(parent, functionNode)) { + return true; + } + + return ( + parent.arguments.at(0) === functionNode && + matchesCalleeName(parent.callee, CALLBACK_HOOK_NAMES) && + grandparent?.type === 'CallExpression' && + isEffectHookCallbackArgument(grandparent, parent) + ); +} + +/** True for a function the enclosing one returns: `return () => {...}`, or an arrow whose body is the function. */ +function isReturnedFunction(functionNode, parent) { + return parent?.type === 'ReturnStatement' || (isFunctionNode(parent) && parent.body === functionNode); +} + +/** The `read` binding of `const read = useMemo(() => () => Onyx.get(key), [])`, for the function the factory returns, or null. */ +function getMemoizedFunctionBinding(functionNode, parent) { + if (!isReturnedFunction(functionNode, parent)) { + return null; + } + + let factory = parent; + + while (factory && !isFunctionNode(factory)) { + factory = factory.parent; + } + + const call = factory?.parent; + const declarator = call?.parent; + + if (call?.type !== 'CallExpression' || call.arguments.at(0) !== factory || getCalleeName(call.callee) !== 'useMemo') { + return null; + } + + return declarator?.type === 'VariableDeclarator' && declarator.init === call && declarator.id.type === 'Identifier' ? {declaration: declarator, name: declarator.id.name} : null; +} + +function getHandlerBinding(functionNode, parent) { + const binding = getFunctionBinding(functionNode, parent); + + return binding ?? getCallbackHookAlias(functionNode); +} + +/** True when the function runs with the code around it: called at once, a synchronous array callback or a Promise executor. For the effect search, a callback passed to `.then()`, `setTimeout()` or another EFFECT_CONTINUATION_NAMES call counts too. */ +function runsWithEnclosingCode(functionNode, parent) { + if (parent?.type === 'NewExpression' && matchesCalleeName(parent.callee, SYNCHRONOUS_EXECUTOR_NAMES)) { + return true; + } + + if (parent?.type !== 'CallExpression') { + return false; + } + + if (parent.callee === functionNode) { + return true; + } + + return ( + parent.arguments.includes(functionNode) && + (matchesCalleeName(parent.callee, EFFECT_CONTINUATION_NAMES) || (parent.callee.type === 'MemberExpression' && matchesCalleeName(parent.callee, SYNCHRONOUS_CALLBACK_METHODS))) + ); +} + +/** True when the read at the innermost ancestor runs inside an effect callback or its cleanup, or in a named handler, possibly through aliases, that an effect calls. */ +function isReachedFromEffect(ancestors, fromIndex, sourceCode, seen) { + function isHandlerInvokedFromEffect(binding) { + return getBindingUses(binding, sourceCode, seen).some((use) => { + const parent = use.parent; + + if (parent?.type === 'CallExpression' && parent.arguments.includes(use) && isEffectCallback(use, parent, parent.parent)) { + return true; + } + + const alias = getAliasBinding(use); + + if (alias) { + return isHandlerInvokedFromEffect(alias); + } + + const isCalled = parent?.type === 'CallExpression' && parent.callee === use; + + if (!isCalled && !runsWithEnclosingCode(use, parent)) { + return false; + } + + const referenceAncestors = sourceCode.getAncestors(use); + + return isReachedFromEffect(referenceAncestors, referenceAncestors.length - 1, sourceCode, seen); + }); + } + + for (let index = fromIndex; index >= 0; index--) { + const ancestor = ancestors[index]; + + if (!isFunctionNode(ancestor)) { + continue; + } + + const parent = ancestors[index - 1] ?? null; + const grandparent = ancestors[index - 2] ?? null; + + if (isEffectCallback(ancestor, parent, grandparent)) { + return true; + } + + const binding = getHandlerBinding(ancestor, parent); + + if (binding) { + return isHandlerInvokedFromEffect(binding); + } + + // A function returned from an effect callback is its cleanup, which React runs from the effect too; a function + // returned from anything else is a value whose call site decides + if (isReturnedFunction(ancestor, parent)) { + const memoized = getMemoizedFunctionBinding(ancestor, parent); + + if (memoized) { + return isHandlerInvokedFromEffect(memoized); + } + + const enclosingIndex = ancestors.findLastIndex((candidate, candidateIndex) => candidateIndex < index && isFunctionNode(candidate)); + + return enclosingIndex !== -1 && isEffectCallback(ancestors[enclosingIndex], ancestors[enclosingIndex - 1] ?? null, ancestors[enclosingIndex - 2] ?? null); + } + + if (!runsWithEnclosingCode(ancestor, parent)) { + return false; + } + } + + return false; +} + +/** True for a file under the allowed directories or listed in READ_ALLOWED_FILES. Also true for a file the rule cannot place in the repo. */ +function isReadAllowedInFile(filename) { + const relativePath = getRepoRelativePath(filename); + + if (!relativePath) { + return true; + } + + return READ_ALLOWED_DIRECTORIES.some((directory) => relativePath.startsWith(directory)) || READ_ALLOWED_FILES.has(relativePath); +} + +/** True for a file under `src/components`, `src/pages` or `src/hooks`, where a read must sit inside a component or hook. Test suites are exempt, because their reads sit in module-level `it` blocks. */ +function requiresComponentOrHookScope(filename) { + const relativePath = getRepoRelativePath(filename); + + return !!relativePath && !relativePath.startsWith(TESTS_DIRECTORY) && READ_ALLOWED_DIRECTORIES.some((directory) => relativePath.startsWith(directory)); +} + +/** True when some enclosing function is named like a component or hook. The function memo() or forwardRef() wraps counts too. */ +function isInsideComponentOrHook(ancestors) { + return ancestors.some((ancestor, index) => { + if (!isFunctionNode(ancestor)) { + return false; + } + + const parent = ancestors[index - 1] ?? null; + + if (isComponentOrHookName(getFunctionName(ancestor, parent))) { + return true; + } + + return parent?.type === 'CallExpression' && parent.arguments.at(0) === ancestor && matchesCalleeName(parent.callee, COMPONENT_WRAPPER_NAMES); + }); +} + +/** + * How the function a read sits in is run. RENDER and MODULE_SCOPE are final. SYNCHRONOUS means it runs with the + * enclosing code, so the caller keeps walking outward. DEFERRED means a handler or other later callback. `classifyCall` + * runs classifyPosition on a call site, so getEscalatedPosition can classify where a helper is called. + */ +function classifyFunctionBoundary(functionNode, parent, sourceCode, seen, classifyCall) { + if (parent?.type === 'Property' && parent.value === functionNode && RENDER_TIME_OPTION_NAMES.has(getStaticName(parent.key, parent.computed)) && isHookOption(parent, sourceCode)) { + return RENDER; + } + + if (parent?.type === 'NewExpression' && parent.arguments.at(0) === functionNode && matchesCalleeName(parent.callee, SYNCHRONOUS_EXECUTOR_NAMES)) { + return SYNCHRONOUS; + } + + if (parent?.type === 'CallExpression') { + if (parent.callee === functionNode) { + return SYNCHRONOUS; + } + + if (parent.arguments.includes(functionNode)) { + if (matchesCalleeName(parent.callee, COMPONENT_WRAPPER_NAMES)) { + return RENDER; + } + + if (getRenderTimeArgumentIndices(parent.callee)?.has(parent.arguments.indexOf(functionNode))) { + return RENDER; + } + + if (parent.callee.type === 'MemberExpression' && matchesCalleeName(parent.callee, SYNCHRONOUS_CALLBACK_METHODS)) { + return SYNCHRONOUS; + } + + // `const read = useCallback(() => Onyx.get(key), []); read();` in a render body + const handler = getHandlerBinding(functionNode, parent); + + return (handler && getEscalatedPosition(handler, sourceCode, seen, classifyCall)) ?? (returnsJSX(functionNode) ? RENDER : DEFERRED); + } + } + + const binding = getFunctionBinding(functionNode, parent) ?? getMemoizedFunctionBinding(functionNode, parent); + const escalated = binding ? getEscalatedPosition(binding, sourceCode, seen, classifyCall) : null; + + if (escalated) { + return escalated; + } + + if (isComponentOrHookName(getFunctionName(functionNode, parent))) { + return RENDER; + } + + return returnsJSX(functionNode) ? RENDER : DEFERRED; +} + +/** + * Where a read with these `ancestors` runs: RENDER, MODULE_SCOPE, EFFECT or EVENT. With `skipEffectCheck`, a deferred + * position returns EVENT without the effect search. Helper call sites only need to know whether they are RENDER or + * MODULE_SCOPE, and the search would otherwise repeat for every call site. + */ +function classifyPosition(ancestors, sourceCode, seen = new Set(), skipEffectCheck = false) { + const classifyCall = (call) => classifyPosition(sourceCode.getAncestors(call), sourceCode, seen, true); + let sawJSXExpression = false; + + for (let index = ancestors.length - 1; index >= 0; index--) { + const ancestor = ancestors[index]; + + if (ancestor.type === 'JSXExpressionContainer') { + sawJSXExpression = true; + continue; + } + + if (!isFunctionNode(ancestor)) { + continue; + } + + if (sawJSXExpression) { + return RENDER; + } + + const disposition = classifyFunctionBoundary(ancestor, ancestors[index - 1] ?? null, sourceCode, seen, classifyCall); + + if (disposition === DEFERRED) { + return !skipEffectCheck && isReachedFromEffect(ancestors, index, sourceCode, new Set()) ? EFFECT : EVENT; + } + + if (disposition === RENDER || disposition === MODULE_SCOPE) { + return disposition; + } + } + + return MODULE_SCOPE; +} + +/** + * True when `value` is assigned, with any operator, to the Onyx property of window or globalThis, as in + * `window.Onyx = Onyx`, `window['Onyx'] = Onyx` or `(window as WindowWithOnyx).Onyx = Onyx as typeof Onyx & {...}`. + * That exposes the library to the dev console, and the rule allows it in every file because it does not hide a read. + */ +function isDevConsoleExposure(value) { + const assignment = value.parent; + + if (assignment?.type !== 'AssignmentExpression' || assignment.right !== value || assignment.left.type !== 'MemberExpression') { + return false; + } + + const target = unwrapTypeOnlyExpressions(assignment.left.object); + + return target.type === 'Identifier' && DEV_CONSOLE_GLOBALS.has(target.name) && getStaticName(assignment.left.property, assignment.left.computed) === DEV_CONSOLE_PROPERTY; +} + +function isObjectValueUse(reference, getReferencedReadMethod) { + const identifier = reference.identifier; + + // Type-only references, `typeof Onyx` and `typeof Onyx.get` read nothing at runtime. @typescript-eslint/scope-manager + // marks the reference in `export {Onyx}` as both type and value (referenceDualValueType in ExportVisitor), so the + // isValueReference check keeps it reported. eslint-scope, used with espree, sets neither flag. + if ((reference.isTypeReference && !reference.isValueReference) || identifier.parent?.type === 'TSTypeQuery' || identifier.parent?.type === 'TSQualifiedName') { + return false; + } + + const value = getOutermostTypeOnlyWrapper(identifier); + const parent = value.parent; + + // A member access, a runtime `typeof Onyx` and the dev console exposure do not hand the object to other code + if ((parent?.type === 'MemberExpression' && parent.object === value) || (parent?.type === 'UnaryExpression' && parent.operator === 'typeof') || isDevConsoleExposure(value)) { + return false; + } + + // `const {get} = Onyx` is reported on the destructured property instead, unless a `...rest` element also takes the other methods + return !( + parent?.type === 'VariableDeclarator' && + parent.init === value && + parent.id.type === 'ObjectPattern' && + parent.id.properties.some((property) => getReferencedReadMethod(property)) && + !parent.id.properties.some((property) => property.type === 'RestElement') + ); +} + +function create(context) { + const sourceCode = context.sourceCode ?? context.getSourceCode(); + const filename = context.filename ?? context.getFilename(); + const shouldCheckComponentOrHookScope = requiresComponentOrHookScope(filename); + const {getCalledReadMethod, getReferencedReadMethod, isOnyxObjectExpression} = createOnyxReadTracker(sourceCode); + const relativePath = getRepoRelativePath(filename); + const shouldCheckObjectValueUses = !TEST_INFRASTRUCTURE_DIRECTORIES.some((directory) => relativePath?.startsWith(directory)); + + // Reported in every file, since the path check below only sees direct calls + function reportReference(node) { + if (!getReferencedReadMethod(node)) { + return; + } + + context.report({node, messageId: 'noOnyxReadByReference'}); + } + + return { + MemberExpression(node) { + // `Onyx[method]` with a key no rule can resolve may name a read method + if (node.computed && getStaticName(node.property, true) === null && isOnyxObjectExpression(node.object, false)) { + context.report({node, messageId: 'noOnyxReadByReference'}); + return; + } + + const callee = getOutermostTypeOnlyWrapper(node); + + if (callee.parent?.type === 'CallExpression' && callee.parent.callee === callee) { + return; + } + + reportReference(node); + }, + 'ObjectPattern > Property': reportReference, + // Walks the import's references through scope analysis, so a local that shadows the import is not reported + ImportDeclaration(node) { + if (!shouldCheckObjectValueUses || node.source.value !== ONYX_MODULE || node.importKind === 'type') { + return; + } + + for (const variable of sourceCode.getDeclaredVariables(node)) { + const specifierType = variable.defs.at(0)?.node.type; + + if (specifierType !== 'ImportDefaultSpecifier' && specifierType !== 'ImportNamespaceSpecifier') { + continue; + } + + for (const reference of variable.references) { + if (isObjectValueUse(reference, getReferencedReadMethod)) { + context.report({node: reference.identifier, messageId: 'noOnyxObjectAsValue'}); + } + } + } + }, + // `export {default as Store} from 'react-native-onyx'` hands out the Onyx object under another name + ExportNamedDeclaration(node) { + if (!shouldCheckObjectValueUses || node.source?.value !== ONYX_MODULE || node.exportKind === 'type') { + return; + } + + for (const specifier of node.specifiers) { + if (specifier.exportKind !== 'type' && getStaticName(specifier.local, false) === 'default') { + context.report({node: specifier, messageId: 'noOnyxObjectAsValue'}); + } + } + }, + // So does `export * as Store from 'react-native-onyx'`, while a bare `export *` leaves the default export out + ExportAllDeclaration(node) { + if (!shouldCheckObjectValueUses || !node.exported || node.source.value !== ONYX_MODULE || node.exportKind === 'type') { + return; + } + + context.report({node, messageId: 'noOnyxObjectAsValue'}); + }, + CallExpression(node) { + if (!getCalledReadMethod(node.callee)) { + return; + } + + if (!isReadAllowedInFile(filename)) { + context.report({node, messageId: 'noOnyxReadOutsideAllowedPath'}); + return; + } + + const ancestors = sourceCode.getAncestors(node); + const position = classifyPosition(ancestors, sourceCode); + + if (position === MODULE_SCOPE) { + context.report({node, messageId: 'noOnyxReadAtModuleScope'}); + return; + } + + if (position === RENDER) { + context.report({node, messageId: 'noOnyxReadInRender'}); + return; + } + + if (position === EFFECT) { + context.report({node, messageId: 'noOnyxReadInEffect'}); + return; + } + + if (shouldCheckComponentOrHookScope && !isInsideComponentOrHook(ancestors)) { + context.report({node, messageId: 'noOnyxReadOutsideComponentOrHook'}); + } + }, + }; +} + +export {name, meta, create}; diff --git a/eslint-plugin-local-rules/utils/onyxReadUtils.js b/eslint-plugin-local-rules/utils/onyxReadUtils.js new file mode 100644 index 000000000000..ae8caa121301 --- /dev/null +++ b/eslint-plugin-local-rules/utils/onyxReadUtils.js @@ -0,0 +1,405 @@ +import fs from 'fs'; +import path from 'path'; + +const ONYX_MODULE = 'react-native-onyx'; + +const SNAPSHOT_KEYS_SOURCE = 'src/CONST/runtimeDefaults.ts'; + +const ONYXKEYS_ROOT = 'ONYXKEYS'; + +const ONYXKEYS_MODULE_SOURCE = /(^|\/)ONYXKEYS$/; + +const READ_METHOD = 'get'; + +const MULTI_READ_METHOD = 'multiGet'; + +const READ_METHODS = new Set([READ_METHOD, MULTI_READ_METHOD]); + +const TYPE_ONLY_EXPRESSIONS = new Set(['TSAsExpression', 'TSSatisfiesExpression', 'TSNonNullExpression', 'TSInstantiationExpression', 'TSTypeAssertion']); + +// This module lives at eslint-plugin-local-rules/utils/, so the repo root is two levels up. The root comes from this +// module's path rather than process.cwd(), so the rules still work when ESLint runs from an editor or a parent workspace. +// Under Jest, config/babel/oxcJestTransformer.js compiles this file to CommonJS with esbuild, which replaces `import.meta` +// with an empty object, so `import.meta.dirname` is undefined there and __dirname is used instead. +const RULES_UTILS_DIR = import.meta.dirname ?? __dirname; + +function findRepoRoot() { + const root = path.resolve(RULES_UTILS_DIR, '../..'); + + return fs.existsSync(path.join(root, SNAPSHOT_KEYS_SOURCE)) ? root : null; +} + +const REPO_ROOT = findRepoRoot(); + +function getStaticName(keyNode, computed) { + if (!computed && keyNode.type === 'Identifier') { + return keyNode.name; + } + + if (keyNode.type === 'Literal' && typeof keyNode.value === 'string') { + return keyNode.value; + } + + // cspell:disable-next-line -- quasis is the ESTree name for the static chunks of a template literal + return keyNode.type === 'TemplateLiteral' && keyNode.expressions.length === 0 ? (keyNode.quasis.at(0)?.value.cooked ?? null) : null; +} + +function getStaticPropertyName(memberExpression) { + return getStaticName(memberExpression.property, memberExpression.computed); +} + +function unwrapTypeOnlyExpressions(node) { + let current = node; + + while (current && TYPE_ONLY_EXPRESSIONS.has(current.type)) { + current = current.expression; + } + + return current; +} + +/** The key expression with casts removed. A template literal that starts with an expression, such as `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, resolves to that leading expression, the collection prefix. */ +function unwrapKeyExpression(node) { + const current = unwrapTypeOnlyExpressions(node); + + if (current?.type !== 'TemplateLiteral') { + return current; + } + + const leadingExpression = current.expressions.at(0); + + // cspell:disable-next-line -- quasis is the ESTree name for the static chunks of a template literal + if (!leadingExpression || current.quasis.at(0)?.value.cooked !== '') { + return current; + } + + return unwrapKeyExpression(leadingExpression); +} + +function getVariableByName(scope, variableName) { + let currentScope = scope; + + while (currentScope) { + const variable = currentScope.set.get(variableName); + + if (variable) { + return variable; + } + + currentScope = currentScope.upper; + } + + return null; +} + +function resolveVariable(identifier, sourceCode) { + return getVariableByName(sourceCode.getScope(identifier), identifier.name); +} + +function getDeclaredVariable(declaration, name, sourceCode) { + return sourceCode.getDeclaredVariables(declaration).find((declaredVariable) => declaredVariable.name === name) ?? null; +} + +function getSingleVariableDefinition(variable, constOnly) { + if (variable?.defs.length !== 1) { + return null; + } + + const definition = variable.defs.at(0); + + if (definition.type !== 'Variable' || (constOnly && definition.parent?.kind !== 'const')) { + return null; + } + + return definition; +} + +function isImportFrom(definition, isSource) { + return definition.type === 'ImportBinding' && definition.parent?.type === 'ImportDeclaration' && isSource(definition.parent.source.value); +} + +/** The initializer of the `const` an identifier resolves to, or null. The lookup starts in the identifier's own scope, so an outer declaration that the inner one shadows is never used. */ +function getConstInitializer(identifier, sourceCode) { + const variable = resolveVariable(identifier, sourceCode); + + if (variable?.defs.length !== 1) { + return null; + } + + const definition = variable.defs.at(0); + + if (definition.type !== 'Variable' || definition.parent?.kind !== 'const' || definition.node.id.type !== 'Identifier') { + return null; + } + + return definition.node.init ?? null; +} + +/** True when `identifier` is the ONYXKEYS default import. Also true for an undeclared global named ONYXKEYS. */ +function isOnyxKeysImport(identifier, sourceCode) { + const variable = resolveVariable(identifier, sourceCode); + + return !variable || variable.defs.every((definition) => isImportFrom(definition, (source) => ONYXKEYS_MODULE_SOURCE.test(source))); +} + +/** The dotted path `node` reads below ONYXKEYS, such as `COLLECTION.REPORT`, or null. A const alias is followed, and a template literal that starts with an ONYXKEYS access resolves to that access. */ +function getOnyxKeyPath(node, sourceCode, seen = new Set()) { + const segments = []; + let current = unwrapKeyExpression(node); + + if (current?.type === 'Identifier' && current.name !== ONYXKEYS_ROOT) { + if (seen.has(current)) { + return null; + } + + seen.add(current); + const initializer = getConstInitializer(current, sourceCode); + + return initializer ? getOnyxKeyPath(initializer, sourceCode, seen) : null; + } + + while (current?.type === 'MemberExpression') { + const propertyName = getStaticPropertyName(current); + + if (!propertyName) { + return null; + } + + segments.unshift(propertyName); + current = current.object; + } + + if (current?.type !== 'Identifier' || current.name !== ONYXKEYS_ROOT || segments.length === 0 || !isOnyxKeysImport(current, sourceCode)) { + return null; + } + + return segments.join('.'); +} + +function getCalleeName(callee) { + if (callee.type === 'Identifier') { + return callee.name; + } + + return callee.type === 'MemberExpression' ? getStaticPropertyName(callee) : null; +} + +function matchesCalleeName(callee, names) { + const calleeName = getCalleeName(callee); + + return !!calleeName && names.has(calleeName); +} + +function getRepoRelativePath(filename) { + if (!REPO_ROOT || !filename || !path.isAbsolute(filename)) { + return null; + } + + const relativePath = path.relative(REPO_ROOT, filename).split(path.sep).join('/'); + + return relativePath.startsWith('..') ? null : relativePath; +} + +/** True when every use of `identifier` is an argument of a call `isAllowedCall` accepts, so no `keys.push()`, `keys[0] = x` or helper that mutates its argument can have changed the elements. */ +function isOnlyPassedTo(identifier, sourceCode, isAllowedCall) { + const variable = resolveVariable(identifier, sourceCode); + + // Other files can import an exported const, and this rule cannot see their uses + if (!variable || variable.defs.some((definition) => definition.parent?.parent?.type === 'ExportNamedDeclaration')) { + return false; + } + + return variable.references.every((reference) => { + // The initializing write, a type reference and a `typeof keys` type query do not use the value at runtime + if (reference.init || reference.isTypeReference || reference.identifier.parent?.type === 'TSTypeQuery') { + return true; + } + + let argument = reference.identifier; + + while (argument.parent && TYPE_ONLY_EXPRESSIONS.has(argument.parent.type)) { + argument = argument.parent; + } + + const call = argument.parent; + + return call?.type === 'CallExpression' && call.arguments.includes(argument) && isAllowedCall(call); + }); +} + +/** The elements of an array literal, written inline or behind a const, or null. A const counts only when it is only ever passed to a call `isAllowedCall` accepts. */ +function getKeyListElements(node, sourceCode, isAllowedCall, seen = new Set()) { + const current = unwrapTypeOnlyExpressions(node); + + if (current?.type === 'Identifier') { + if (seen.has(current) || !isOnlyPassedTo(current, sourceCode, isAllowedCall)) { + return null; + } + + seen.add(current); + const initializer = getConstInitializer(current, sourceCode); + + return initializer ? getKeyListElements(initializer, sourceCode, isAllowedCall, seen) : null; + } + + return current?.type === 'ArrayExpression' ? current.elements : null; +} + +/** + * Returns functions that tell whether a callee or reference is `Onyx.get` or `Onyx.multiGet` from `react-native-onyx`. + * They follow any chain of aliases in the file, such as `import O from 'react-native-onyx'`, `const api = Onyx`, + * `const {get: read} = Onyx`, `const read = Onyx.get` or `Onyx.default` on a namespace import, and strip TS casts at + * every step. They walk variable definitions rather than visitor order, so an alias declared after its first use is + * still found. With `constOnly`, an alias bound by `let` or `var` does not count. The Concierge exemption in + * no-onyx-get-snapshot-key needs that, because such a binding could be reassigned. + */ +function createOnyxReadTracker(sourceCode) { + const memo = new Map(); + + function isOnyxModuleSource(source) { + return source === ONYX_MODULE; + } + + /** Caches the answer to `question` about `variable`. One slot per question and per `constOnly`, so an answer computed with one strictness is never returned for the other. */ + function memoized(variable, question, constOnly, compute) { + const key = `${question}:${constOnly ? 'const' : 'any'}`; + let entry = memo.get(variable); + + if (!entry) { + entry = {}; + memo.set(variable, entry); + } + + if (!(key in entry)) { + // The null seed makes a self-referential chain read as "not Onyx" instead of recursing without end + entry[key] = null; + entry[key] = compute(); + } + + return entry[key]; + } + + function getImportSpecifier(identifier, isSource) { + const variable = resolveVariable(identifier, sourceCode); + + return !!variable && variable.defs.length === 1 && isImportFrom(variable.defs.at(0), isSource) ? variable.defs.at(0).node : null; + } + + /** True when `node` is the Onyx import or a const alias of it. Also true for `Onyx.default` on a namespace import. */ + function isOnyxObjectExpression(node, constOnly) { + const current = unwrapTypeOnlyExpressions(node); + + if (current?.type === 'MemberExpression') { + const namespace = unwrapTypeOnlyExpressions(current.object); + + return getStaticPropertyName(current) === 'default' && namespace?.type === 'Identifier' && getImportSpecifier(namespace, isOnyxModuleSource)?.type === 'ImportNamespaceSpecifier'; + } + + const variable = current?.type === 'Identifier' ? resolveVariable(current, sourceCode) : null; + + if (!variable) { + return false; + } + + return !!memoized(variable, 'object', constOnly, () => { + if (variable.defs.length === 1 && isImportFrom(variable.defs.at(0), isOnyxModuleSource)) { + const specifier = variable.defs.at(0).node; + return specifier.type === 'ImportDefaultSpecifier' || specifier.type === 'ImportNamespaceSpecifier'; + } + + const definition = getSingleVariableDefinition(variable, constOnly); + + return definition?.node.id.type === 'Identifier' && isOnyxObjectExpression(definition.node.init, constOnly); + }); + } + + function getMemberReadMethod(node, constOnly) { + const member = unwrapTypeOnlyExpressions(node); + + if (member?.type !== 'MemberExpression') { + return null; + } + + const propertyName = getStaticPropertyName(member); + + return READ_METHODS.has(propertyName) && isOnyxObjectExpression(member.object, constOnly) ? propertyName : null; + } + + /** The read method an identifier is bound to through `const read = Onyx.get`, `const read2 = read` or `const {get: read} = Onyx`, or null. */ + function getIdentifierReadMethod(identifier, constOnly) { + const variable = resolveVariable(identifier, sourceCode); + const definition = getSingleVariableDefinition(variable, constOnly); + + if (!definition) { + return null; + } + + return memoized(variable, 'method', constOnly, () => { + const {id, init} = definition.node; + + if (id.type === 'Identifier') { + const value = unwrapTypeOnlyExpressions(init); + + // `const read2 = read` follows the alias; anything else must be the member access itself + return value?.type === 'Identifier' ? getIdentifierReadMethod(value, constOnly) : getMemberReadMethod(value, constOnly); + } + + if (id.type !== 'ObjectPattern' || !isOnyxObjectExpression(init, constOnly)) { + return null; + } + + const property = id.properties.find((candidate) => candidate.type === 'Property' && candidate.value.type === 'Identifier' && candidate.value.name === identifier.name); + const keyName = property ? getStaticName(property.key, property.computed) : null; + + return READ_METHODS.has(keyName) ? keyName : null; + }); + } + + /** Returns `get`, `multiGet` or null. */ + function getCalledReadMethod(callee, {constOnly = false} = {}) { + const current = unwrapTypeOnlyExpressions(callee); + + if (current?.type === 'Identifier') { + return getIdentifierReadMethod(current, constOnly); + } + + return getMemberReadMethod(current, constOnly); + } + + /** The read method a member access (`Onyx.get`) or a destructured property (`const {get} = Onyx`) names, or null. */ + function getReferencedReadMethod(node) { + if (node.type !== 'Property' || node.parent?.type !== 'ObjectPattern') { + return getMemberReadMethod(node, false); + } + + const declarator = node.parent.parent; + const isFromOnyx = declarator?.type === 'VariableDeclarator' && declarator.id === node.parent && isOnyxObjectExpression(declarator.init, false); + const keyName = getStaticName(node.key, node.computed); + + return READ_METHODS.has(keyName) && isFromOnyx ? keyName : null; + } + + return {getCalledReadMethod, getReferencedReadMethod, getImportSpecifier, isOnyxObjectExpression}; +} + +export { + ONYX_MODULE, + ONYXKEYS_ROOT, + REPO_ROOT, + SNAPSHOT_KEYS_SOURCE, + READ_METHOD, + MULTI_READ_METHOD, + TYPE_ONLY_EXPRESSIONS, + getStaticName, + unwrapTypeOnlyExpressions, + resolveVariable, + getDeclaredVariable, + getSingleVariableDefinition, + getConstInitializer, + getOnyxKeyPath, + getCalleeName, + matchesCalleeName, + getRepoRelativePath, + getKeyListElements, + createOnyxReadTracker, +}; diff --git a/package-lock.json b/package-lock.json index b444f4d02b72..fc3f90f2f8e7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -126,7 +126,7 @@ "react-native-nitro-fetch": "1.6.2", "react-native-nitro-modules": "0.37.1", "react-native-nitro-sqlite": "9.8.3", - "react-native-onyx": "3.0.117", + "react-native-onyx": "3.0.119", "react-native-pager-view": "9.0.4", "react-native-pdf": "7.0.2", "react-native-permissions": "^5.4.0", @@ -37320,9 +37320,9 @@ } }, "node_modules/react-native-onyx": { - "version": "3.0.117", - "resolved": "https://registry.npmjs.org/react-native-onyx/-/react-native-onyx-3.0.117.tgz", - "integrity": "sha512-0gidHoohos8p6746M03GtL+l8kA8kWM/E+9Cq0/wHFcsP8S1Qg68qIcRXYlgrdO8WBbl7r4hhKKqK2+ZB2ox6w==", + "version": "3.0.119", + "resolved": "https://registry.npmjs.org/react-native-onyx/-/react-native-onyx-3.0.119.tgz", + "integrity": "sha512-1idaVW/nP0jTSjjevAzzMlk6gOLqkmT6gJaDYI0fWx21vtQLJ0gZeMEJb16g08Bd9nY4bsALWb4bkzhmiLWIfw==", "license": "MIT", "dependencies": { "ascii-table": "0.0.9", diff --git a/package.json b/package.json index b20926ce6949..dcc1d21ea0d3 100644 --- a/package.json +++ b/package.json @@ -203,7 +203,7 @@ "react-native-nitro-fetch": "1.6.2", "react-native-nitro-modules": "0.37.1", "react-native-nitro-sqlite": "9.8.3", - "react-native-onyx": "3.0.117", + "react-native-onyx": "3.0.119", "react-native-pager-view": "9.0.4", "react-native-pdf": "7.0.2", "react-native-permissions": "^5.4.0", diff --git a/scripts/checkOnyxConnectBypass.ts b/scripts/checkOnyxConnectBypass.ts deleted file mode 100644 index 7134732ade19..000000000000 --- a/scripts/checkOnyxConnectBypass.ts +++ /dev/null @@ -1,93 +0,0 @@ -#!/usr/bin/env bun - -import {file} from 'bun'; -/** - * Fails the lint run when a new inline `eslint-disable` bypasses the Onyx.connect() ban. - * - * The ban (`rulesdir/no-onyx-connect`, shipped by eslint-config-expensify) is a normal lint rule, - * so an inline disable can silence it. The runner re-elevates those disables by scanning source - * for directives that name the ban or blanket directives that cover a real call — no disable - * comment can reach this check. - * - * A real bypass requires a file to mention `Onyx` and `connect` and contain an `eslint-disable` - * directive, so we first narrow the targets to files matching all three (via git grep). The - * candidate scan does not require the contiguous text `Onyx.connect` — git grep is line-oriented, so a - * spaced or split `Onyx . connect(` would otherwise be skipped. Extra matches like - * `Onyx.connectWithoutView` are harmless: we only fail on disable directives that actually - * suppress the ban. - */ -import {execFileSync} from 'node:child_process'; -import path from 'node:path'; - -import {collectDisableDirectivesFromSource, findNewBypasses} from './onyxConnectBypass'; - -const projectRoot = path.resolve(import.meta.dir, '..'); - -/** Files among the lint targets that mention Onyx, connect, and eslint-disable. */ -function findCandidateFiles(targets: string[]): string[] { - const pathSpecs = targets.length > 0 ? targets : ['.']; - try { - const output = execFileSync( - 'git', - ['grep', '-lI', '--all-match', '--untracked', '--no-recurse-submodules', '-e', 'Onyx', '-e', 'connect', '-e', 'eslint-disable', '--', ...pathSpecs], - { - cwd: projectRoot, - encoding: 'utf8', - }, - ); - return output.split('\n').filter(Boolean); - } catch (error: unknown) { - if (typeof error === 'object' && error !== null && 'status' in error && error.status === 1) { - return []; - } - throw error; - } -} - -/** - * Checks `targets` for new Onyx.connect() ban bypasses, reporting any to stderr. - * Returns `true` if a new bypass was found (i.e. the caller should fail). - */ -async function checkOnyxConnectBypass(targets: string[]): Promise { - const candidates = findCandidateFiles(targets); - if (candidates.length === 0) { - return false; - } - - const suppressed = ( - await Promise.all( - candidates.map(async (relativePath) => { - const source = await file(path.join(projectRoot, relativePath)).text(); - return collectDisableDirectivesFromSource(source, relativePath.split(path.sep).join('/')); - }), - ) - ).flat(); - - const newBypasses = findNewBypasses(suppressed); - if (newBypasses.length === 0) { - return false; - } - - console.error('Onyx.connect() is banned and the ban cannot be bypassed with eslint-disable. Use the useOnyx() hook to read Onyx data instead.'); - console.error('New bypasses found:'); - for (const bypass of newBypasses) { - console.error(` ${bypass.file}:${bypass.line}`); - } - return true; -} - -if (import.meta.main) { - checkOnyxConnectBypass(process.argv.slice(2)) - .then((failed) => { - if (!failed) { - return; - } - process.exitCode = 1; - }) - .catch((error: unknown) => { - console.error(error instanceof Error ? error.message : error); - process.exitCode = 1; - }); -} - -export default checkOnyxConnectBypass; diff --git a/scripts/checkOnyxLintBypass.ts b/scripts/checkOnyxLintBypass.ts new file mode 100644 index 000000000000..08e2bd743dd5 --- /dev/null +++ b/scripts/checkOnyxLintBypass.ts @@ -0,0 +1,122 @@ +#!/usr/bin/env bun + +import {file} from 'bun'; +/** + * Fails the lint run when a new inline `eslint-disable` or `eslint` config comment silences one of + * the Onyx lint bans in `BANNED_RULES` (see `onyxLintBypass.ts`). Every ban there is a normal lint + * rule, so ESLint honors an inline comment that turns it off. This script reads the source itself and + * reports directives that name the ban, blanket directives over a real banned use, and config comments + * that name the ban. For the OnyxUtils ban a named directive or config comment counts only when the + * file has a runtime import. The script runs outside ESLint, so no disable comment can turn it off. + * + * A bypass needs an `eslint` comment and the ban's search terms in the same file, so git grep first + * narrows the targets to files that contain all of them. The terms are separate words rather than + * `Onyx.connect`, because git grep matches one line at a time and would miss a call split across lines + * or spaced as `Onyx . connect(`. Extra matches such as `Onyx.connectWithoutView` are harmless, since + * only a directive that silences a real use fails the run. + * + * It also fails when the seatbelt TSV has a row for a rule in `SEATBELT_FORBIDDEN_RULES`, since such a row allows the + * rule's errors the same way a disable comment would. A row is not tied to the lint targets, so this check runs on every call. + */ +import {execFileSync} from 'node:child_process'; +import path from 'node:path'; + +import type {BannedRule} from './onyxLintBypass'; + +import {BANNED_RULES, collectDisableDirectivesFromSource, findForbiddenSeatbeltRows, findNewBypasses, SEATBELT_FORBIDDEN_RULES} from './onyxLintBypass'; + +const projectRoot = path.resolve(import.meta.dir, '..'); + +const SEATBELT_TSV = 'config/eslint/eslint.seatbelt.tsv'; + +/** Repo-relative paths of the files under `targets` that contain every search term. An empty `targets` searches the whole repo. */ +function findCandidateFiles(targets: string[], searchTerms: string[]): string[] { + const pathSpecs = targets.length > 0 ? targets : ['.']; + try { + const output = execFileSync('git', ['grep', '-lI', '--all-match', '--untracked', '--no-recurse-submodules', ...searchTerms.flatMap((term) => ['-e', term]), '--', ...pathSpecs], { + cwd: projectRoot, + encoding: 'utf8', + }); + return output.split('\n').filter(Boolean); + } catch (error: unknown) { + // git grep exits with status 1 when no file matches + if (typeof error === 'object' && error !== null && 'status' in error && error.status === 1) { + return []; + } + throw error; + } +} + +/** Prints each new bypass to stderr and resolves to `true` when there is one. */ +async function checkBan(targets: string[], ban: BannedRule): Promise { + const candidates = findCandidateFiles(targets, ban.searchTerms) + .map((relativePath) => relativePath.split(path.sep).join('/')) + .filter(ban.appliesTo); + if (candidates.length === 0) { + return false; + } + + const suppressed = ( + await Promise.all( + candidates.map(async (relativePath) => { + const source = await file(path.join(projectRoot, relativePath)).text(); + return collectDisableDirectivesFromSource(source, relativePath, ban); + }), + ) + ).flat(); + + const newBypasses = findNewBypasses(suppressed, ban); + if (newBypasses.length === 0) { + return false; + } + + console.error(ban.message); + console.error('New bypasses found:'); + for (const bypass of newBypasses) { + console.error(` ${bypass.file}:${bypass.line}`); + } + return true; +} + +/** Prints each forbidden row to stderr and resolves to `true` when there is one. A missing TSV passes. */ +async function checkSeatbelt(): Promise { + const tsvFile = file(path.join(projectRoot, SEATBELT_TSV)); + if (!(await tsvFile.exists())) { + return false; + } + const forbiddenRows = findForbiddenSeatbeltRows(await tsvFile.text(), SEATBELT_FORBIDDEN_RULES); + if (forbiddenRows.length === 0) { + return false; + } + + console.error( + `${SEATBELT_TSV} has a row for ${SEATBELT_FORBIDDEN_RULES.join(' or ')}. A seatbelt row allows the rule's errors in that file, the same as a disable comment. Remove the row and fix the reads it covers; SEATBELT_INCREASE must not be used for these rules.`, + ); + console.error('Forbidden seatbelt rows found:'); + for (const row of forbiddenRows) { + console.error(` ${SEATBELT_TSV}:${row.line} ${path.posix.normalize(path.posix.join(path.posix.dirname(SEATBELT_TSV), row.file))} ${row.rule}`); + } + return true; +} + +async function checkOnyxLintBypass(targets: string[]): Promise { + const results = await Promise.all([...BANNED_RULES.map((ban) => checkBan(targets, ban)), checkSeatbelt()]); + return results.some(Boolean); +} + +if (import.meta.main) { + checkOnyxLintBypass(process.argv.slice(2)) + .then((failed) => { + if (!failed) { + return; + } + process.exitCode = 1; + }) + .catch((error: unknown) => { + console.error(error instanceof Error ? error.message : error); + process.exitCode = 1; + }); +} + +export default checkOnyxLintBypass; +export {findCandidateFiles}; diff --git a/scripts/checkOnyxLintDisableReason.ts b/scripts/checkOnyxLintDisableReason.ts new file mode 100644 index 000000000000..23595e1743dc --- /dev/null +++ b/scripts/checkOnyxLintDisableReason.ts @@ -0,0 +1,67 @@ +#!/usr/bin/env bun + +import {file} from 'bun'; +/** + * Fails the lint run when an inline `eslint-disable` of one of the rules in `REASONED_RULES` (see + * `onyxLintDisableReason.ts`) gives no reason, or an `eslint` config comment names one of them. + * git grep narrows the candidate files the same way it does in `checkOnyxLintBypass.ts`, then the + * script checks each comment Babel finds. + */ +import path from 'node:path'; + +import type {ReasonedRule} from './onyxLintDisableReason'; + +import {findCandidateFiles} from './checkOnyxLintBypass'; +import {REASONED_RULES, collectUnreasonedDisables} from './onyxLintDisableReason'; + +const projectRoot = path.resolve(import.meta.dir, '..'); + +/** Prints each disable without a reason to stderr and resolves to `true` when there is one. */ +async function checkRule(targets: string[], rule: ReasonedRule): Promise { + const candidates = findCandidateFiles(targets, rule.searchTerms) + .map((relativePath) => relativePath.split(path.sep).join('/')) + .filter(rule.appliesTo); + if (candidates.length === 0) { + return false; + } + + const unreasoned = ( + await Promise.all( + candidates.map(async (relativePath) => { + const source = await file(path.join(projectRoot, relativePath)).text(); + return collectUnreasonedDisables(source, relativePath, rule); + }), + ) + ).flat(); + if (unreasoned.length === 0) { + return false; + } + + console.error(rule.message); + console.error('Disables without a reason found:'); + for (const disable of unreasoned) { + console.error(` ${disable.file}:${disable.line}`); + } + return true; +} + +async function checkOnyxLintDisableReason(targets: string[]): Promise { + const results = await Promise.all(REASONED_RULES.map((rule) => checkRule(targets, rule))); + return results.some(Boolean); +} + +if (import.meta.main) { + checkOnyxLintDisableReason(process.argv.slice(2)) + .then((failed) => { + if (!failed) { + return; + } + process.exitCode = 1; + }) + .catch((error: unknown) => { + console.error(error instanceof Error ? error.message : error); + process.exitCode = 1; + }); +} + +export default checkOnyxLintDisableReason; diff --git a/scripts/lint/index.ts b/scripts/lint/index.ts index ca7fe1fb25a0..e8381f7d57c9 100644 --- a/scripts/lint/index.ts +++ b/scripts/lint/index.ts @@ -11,7 +11,8 @@ import CLI from 'expensify-common/CLI'; -import checkOnyxConnectBypass from '../checkOnyxConnectBypass'; +import checkOnyxLintBypass from '../checkOnyxLintBypass'; +import checkOnyxLintDisableReason from '../checkOnyxLintDisableReason'; import Bench from '../utils/Bench'; import ESLintLinter from './eslint/ESLintLinter'; import StylishFormatter from './formatters/StylishFormatter'; @@ -84,6 +85,8 @@ if (result.exitCode !== 0) { process.exit(result.exitCode); } -if (await checkOnyxConnectBypass(lintTargets)) { +const onyxChecksFailed = await Promise.all([checkOnyxLintBypass(lintTargets), checkOnyxLintDisableReason(lintTargets)]); + +if (onyxChecksFailed.some(Boolean)) { process.exit(1); } diff --git a/scripts/onyxConnectBypass.ts b/scripts/onyxConnectBypass.ts deleted file mode 100644 index 993200680ad7..000000000000 --- a/scripts/onyxConnectBypass.ts +++ /dev/null @@ -1,270 +0,0 @@ -/** - * Detection logic for new `eslint-disable` bypasses of the Onyx.connect() ban. - * - * `rulesdir/no-onyx-connect` (shipped by eslint-config-expensify) is a normal lint rule, so an - * inline `eslint-disable` can silence it. The lint runner re-elevates those disables by scanning - * source for disable directives that name the ban or blanket directives that cover a real call. No - * disable directive can reach this check because it does not go through ESLint's message pipeline. - * - * Blanket `eslint-disable` / `eslint-disable-next-line` with no rule list counts only when it - * covers a real Onyx.connect() call. Unrelated blanket comments (e.g. around ReportUtils) remain - * ignored. Call sites are found via the Babel AST so comments and grouping parens cannot hide a - * banned member access from a source scan. - */ - -import {parse} from '@babel/parser'; - -import type {ASTNode} from './utils/BabelASTUtils'; - -import BabelASTUtils from './utils/BabelASTUtils'; - -/** Rule id of the Onyx.connect() ban, as exposed through eslint-plugin-rulesdir. */ -const BANNED_RULE_ID = 'rulesdir/no-onyx-connect'; - -const BANNED_RULE_NAME = 'no-onyx-connect'; - -/** - * Disables of the ban that already exist on `main`, keyed by repo-relative path with the number of - * occurrences in each file. Migrating these call sites to useOnyx() is already in progress; any - * suppressed violation beyond these counts is treated as a new bypass. - */ -const GRANDFATHERED_BYPASSES = new Map([ - ['src/libs/NextStepUtils.ts', 1], - ['src/libs/ReportNameUtils.ts', 2], -]); - -/** A `no-onyx-connect` violation that an inline disable directive silenced. */ -type SuppressedBan = { - file: string; - line: number; -}; - -type DirectiveMatch = { - index: number; - text: string; - kind?: string; - args: string; -}; - -type BabelComment = { - type: string; - value: string; - start: number | null; - end: number | null; -}; - -function parseSource(source: string): {root: ASTNode; comments: BabelComment[]} | null { - try { - const parsed: unknown = parse(source, {sourceType: 'unambiguous', plugins: ['typescript', 'jsx'], errorRecovery: true, attachComment: true}); - if (!BabelASTUtils.isASTNode(parsed) || !BabelASTUtils.isRecord(parsed)) { - return null; - } - const rawComments = parsed.comments; - const comments = Array.isArray(rawComments) - ? rawComments.filter((comment): comment is BabelComment => { - return ( - BabelASTUtils.isRecord(comment) && - typeof comment.type === 'string' && - typeof comment.value === 'string' && - typeof comment.start === 'number' && - typeof comment.end === 'number' - ); - }) - : []; - return {root: parsed, comments}; - } catch { - return null; - } -} - -function collectDirectiveMatches(comments: readonly BabelComment[], source: string, directive: 'disable' | 'enable'): DirectiveMatch[] { - const matches: DirectiveMatch[] = []; - for (const comment of comments) { - if (comment.start === null || comment.end === null) { - continue; - } - // ESLint only supports -line / -next-line on disable. `eslint-enable-line` and - // `eslint-enable-next-line` are not real directives, so treating them as enables - // would reopen a blanket disable that ESLint still honors. - const pattern = directive === 'disable' ? '^\\s*eslint-disable(?-next-line|-line)?(?[\\s\\S]*)$' : '^\\s*eslint-enable(?!-)(?[\\s\\S]*)$'; - const directiveMatch = comment.value.match(new RegExp(pattern)); - if (!directiveMatch) { - continue; - } - matches.push({index: comment.start, text: source.slice(comment.start, comment.end), kind: directiveMatch.groups?.kind, args: directiveMatch.groups?.args ?? ''}); - } - return matches; -} - -function directiveKind(match: DirectiveMatch): string | undefined { - return match.kind; -} - -function directiveArgs(match: DirectiveMatch): string { - return match.args; -} - -const NON_CHILD_KEYS = new Set(['loc', 'start', 'end', 'extra', 'leadingComments', 'trailingComments', 'innerComments', 'comments']); -const WRAPPER_TYPES = new Set(['ParenthesizedExpression', 'TSAsExpression', 'TSSatisfiesExpression', 'TSNonNullExpression', 'TSTypeAssertion']); - -function unwrapExpression(node: ASTNode): ASTNode { - let current = node; - while (WRAPPER_TYPES.has(current.type) && BabelASTUtils.isASTNode(current.expression)) { - current = current.expression; - } - return current; -} - -function isOnyxConnectCall(node: ASTNode): boolean { - // Optional chaining anywhere in the call (`Onyx?.connect(...)`, `Onyx.connect?.(...)`) produces - // `OptionalCallExpression`/`OptionalMemberExpression` nodes instead of their non-optional - // counterparts, so a blanket disable directive over one would otherwise silently bypass the ban. - if ((node.type !== 'CallExpression' && node.type !== 'OptionalCallExpression') || !BabelASTUtils.isASTNode(node.callee)) { - return false; - } - const callee = node.callee; - if ((callee.type !== 'MemberExpression' && callee.type !== 'OptionalMemberExpression') || callee.computed === true) { - return false; - } - if (!BabelASTUtils.isASTNode(callee.property) || callee.property.type !== 'Identifier' || callee.property.name !== 'connect') { - return false; - } - if (!BabelASTUtils.isASTNode(callee.object)) { - return false; - } - const object = unwrapExpression(callee.object); - return object.type === 'Identifier' && object.name === 'Onyx'; -} - -function collectOnyxConnectCallOffsets(root: ASTNode): number[] { - const offsets: number[] = []; - const visit = (node: ASTNode) => { - if (isOnyxConnectCall(node)) { - offsets.push(node.start); - } - for (const child of BabelASTUtils.children(node, NON_CHILD_KEYS)) { - visit(child); - } - }; - visit(root); - return offsets; -} - -function normalizedDirectiveArgs(args: string): string { - return args - .replace(/--[\s\S]*$/, '') - .replaceAll(/[\s*]+/g, ' ') - .trim(); -} - -function directiveTargetsBan(args: string): boolean { - const trimmed = normalizedDirectiveArgs(args); - if (trimmed.length === 0) { - return false; - } - return trimmed.split(',').some((part) => { - const rule = part.trim(); - return rule === BANNED_RULE_ID || rule === BANNED_RULE_NAME || rule.endsWith(`/${BANNED_RULE_NAME}`); - }); -} - -function isBlanketDirective(args: string): boolean { - return normalizedDirectiveArgs(args).length === 0; -} - -function lineNumberAtOffset(source: string, offset: number): number { - let line = 1; - let index = 0; - const end = Math.min(Math.max(offset, 0), source.length); - while (index < end) { - const code = source.charCodeAt(index); - if (code === 0x0d) { - line++; - index += source.charCodeAt(index + 1) === 0x0a ? 2 : 1; - continue; - } - if (code === 0x0a || code === 0x2028 || code === 0x2029) { - line++; - } - index++; - } - return line; -} - -function blanketDirectiveCoversCall(source: string, match: DirectiveMatch, callOffsets: number[], enableMatches: DirectiveMatch[]): boolean { - const directiveLine = lineNumberAtOffset(source, match.index); - const kind = directiveKind(match); - const directiveEnd = match.index + match.text.length; - const directiveEndLine = lineNumberAtOffset(source, Math.max(match.index, directiveEnd - 1)); - return callOffsets.some((callOffset) => { - const callLine = lineNumberAtOffset(source, callOffset); - if (kind === '-line') { - return callLine === directiveLine; - } - if (kind === '-next-line') { - return callLine === directiveEndLine + 1; - } - if (callOffset <= directiveEnd) { - return false; - } - const reenabled = enableMatches.some((enableMatch) => { - const enableOffset = enableMatch.index; - if (enableOffset <= directiveEnd || enableOffset >= callOffset) { - return false; - } - const enableArgs = directiveArgs(enableMatch); - return isBlanketDirective(enableArgs) || directiveTargetsBan(enableArgs); - }); - return !reenabled; - }); -} - -/** - * Find disable directives in `source` that suppress `rulesdir/no-onyx-connect`. - * Line numbers are 1-based. Matches both full-line and trailing `eslint-disable-line`. - */ -function collectDisableDirectivesFromSource(source: string, file: string): SuppressedBan[] { - const parsed = parseSource(source); - if (!parsed) { - return []; - } - const bans: SuppressedBan[] = []; - const callOffsets = collectOnyxConnectCallOffsets(parsed.root); - const enableMatches = collectDirectiveMatches(parsed.comments, source, 'enable'); - for (const match of collectDirectiveMatches(parsed.comments, source, 'disable')) { - const args = directiveArgs(match); - const targetsBan = directiveTargetsBan(args); - const coversBan = isBlanketDirective(args) && blanketDirectiveCoversCall(source, match, callOffsets, enableMatches); - if (!targetsBan && !coversBan) { - continue; - } - const prefix = source.slice(0, match.index); - const line = prefix.split('\n').length; - bans.push({file, line}); - } - return bans; -} - -/** Return the suppressed bans that exceed the grandfathered allowance for their file. */ -function findNewBypasses(suppressedBans: readonly SuppressedBan[]): SuppressedBan[] { - const byFile = new Map(); - for (const ban of suppressedBans) { - const list = byFile.get(ban.file) ?? []; - list.push(ban); - byFile.set(ban.file, list); - } - - const newBypasses: SuppressedBan[] = []; - for (const [file, bans] of byFile) { - const allowed = GRANDFATHERED_BYPASSES.get(file) ?? 0; - if (bans.length <= allowed) { - continue; - } - const sortedByLine = [...bans].sort((a, b) => a.line - b.line); - newBypasses.push(...sortedByLine.slice(allowed)); - } - return newBypasses; -} - -export {BANNED_RULE_ID, BANNED_RULE_NAME, GRANDFATHERED_BYPASSES, collectDisableDirectivesFromSource, findNewBypasses}; -export type {SuppressedBan}; diff --git a/scripts/onyxLintBypass.ts b/scripts/onyxLintBypass.ts new file mode 100644 index 000000000000..a4d3b312f1ba --- /dev/null +++ b/scripts/onyxLintBypass.ts @@ -0,0 +1,805 @@ +/** + * Finds `eslint-disable` directives and `eslint` config comments that silence one of the Onyx lint bans in `BANNED_RULES`: + * `rulesdir/no-onyx-connect`, `rulesdir/no-unsafe-onyx-read`, and the + * `@typescript-eslint/no-restricted-imports` entry for `react-native-onyx/dist/OnyxUtils`. + * None of these may be disabled inline. Directives beyond each ban's grandfathered count fail the lint run. + * + * A directive counts when it names the ban's rule, or when it is a blanket disable over a banned + * use. For the connect ban that use is `Onyx.connect`. For the read ban it is any reference to `Onyx.get` or + * `Onyx.multiGet`, or the Onyx object itself used as a value (`const api = {...Onyx}`). + * For the OnyxUtils ban a directive counts only when it covers a runtime import of the module. Blanket + * disables over unrelated code, like the ones around ReportUtils, are ignored. A file Babel cannot + * parse fails closed: every disable in it counts. + * + * The script finds calls and imports in the Babel AST, so spacing, comments and parentheses that would + * fool a text scan cannot hide them. + * + * A seatbelt TSV row also turns a rule's errors into allowed ones, so the two Onyx read rules in + * `SEATBELT_FORBIDDEN_RULES` may not have a row there at all. + */ + +import type {ParserPlugin} from '@babel/parser'; + +import {parse} from '@babel/parser'; + +import type {ASTNode} from './utils/BabelASTUtils'; + +import BabelASTUtils from './utils/BabelASTUtils'; + +const BANNED_RULE_ID = 'rulesdir/no-onyx-connect'; + +const BANNED_RULE_NAME = 'no-onyx-connect'; + +const ONYX_MODULE = 'react-native-onyx'; + +/** Inline disables of no-onyx-connect that already existed on `main`, by file path and count. A file with more than its count fails the check. */ +const GRANDFATHERED_BYPASSES = new Map([ + ['src/libs/NextStepUtils.ts', 1], + ['src/libs/ReportNameUtils.ts', 2], +]); + +type BannedRule = { + id: string; + name: string; + objects: Set; + methods: Set; + /** Bans runtime imports of these modules instead of `objects`/`methods` calls. */ + importSources?: Set; + /** + * The rule reports the member access itself as well as a call, so a line that holds `Onyx.connect` or `Onyx.get` + * as a value (`const read = Onyx.get`, `keys.map(Onyx.get)`, `Onyx.get.call(...)`) or takes it by destructuring + * (`const {get} = Onyx`) is a use too. Calls, including calls through an alias, count for every ban. no-onyx-connect + * reports neither alias calls nor destructuring, so counting them for it is part of the name-based superset described + * at `collectBannedNames`. + */ + reportsOnMember?: boolean; + /** + * The rule reports the banned object itself used as a value (`const api = {...Onyx}`, `fn(Onyx)`, `return Onyx`), so a + * blanket directive over such a line silences it. The rule allows one value use, the dev console exposure: an assignment + * to the Onyx property of window or globalThis, cast or not (`(window as X).Onyx = Onyx`). It skips value uses in test + * infrastructure. In every file it also reports a member whose key it cannot resolve (`Onyx[method]`), since that key + * may name a read method. A runtime re-export of the object (`export {default as Store} from 'react-native-onyx'`, + * `export * as Store from ...`) is a value use too. + */ + reportsOnObjectValue?: boolean; + /** + * A directive on an inner line of a multi-line banned call, such as an element line of `Onyx.multiGet([...])`, + * counts too. The read rule reports a call on its first line, but a directive inside the call is still an + * attempt to silence the read. + */ + coversInnerLines?: boolean; + grandfathered: Map; + appliesTo: (file: string) => boolean; + searchTerms: string[]; + message: string; +}; + +/** True for a file ESLint lints. A directive quoted in a markdown doc or a patch file silences nothing, so those files are skipped. */ +function isLintedFile(file: string): boolean { + return /\.[cm]?[jt]sx?$/.test(file); +} + +/** True for a linted file outside `tests/`. Test suites may read Onyx, and the rule suites quote banned code as fixtures. */ +function isCheckedSourceFile(file: string): boolean { + return isLintedFile(file) && !file.startsWith('tests/'); +} + +// Directories where no-unsafe-onyx-read skips value uses of the Onyx object, such as the `...Onyx` in __mocks__/react-native-onyx.ts. +// Its path check still reports reads in `__mocks__/` and `jest/`, so calls in those files stay checked +const TEST_INFRASTRUCTURE_DIRECTORIES = ['tests/', '__mocks__/', 'jest/']; + +function isTestInfrastructureFile(file: string): boolean { + return TEST_INFRASTRUCTURE_DIRECTORIES.some((directory) => file.startsWith(directory)); +} + +const ONYX_CONNECT_BAN: BannedRule = { + id: BANNED_RULE_ID, + name: BANNED_RULE_NAME, + objects: new Set(['Onyx']), + methods: new Set(['connect']), + reportsOnMember: true, + grandfathered: GRANDFATHERED_BYPASSES, + appliesTo: isLintedFile, + // `eslint` rather than `eslint-disable`, so a file whose only directive is a config comment is a candidate + searchTerms: ['Onyx', 'connect', 'eslint'], + message: + 'An eslint-disable or eslint config comment silences rulesdir/no-onyx-connect. Onyx.connect() is deprecated, and its ban allows no new inline exceptions. Remove the comment and read the data with useOnyx().', +}; + +const ONYX_READ_BAN: BannedRule = { + id: 'rulesdir/no-unsafe-onyx-read', + name: 'no-unsafe-onyx-read', + objects: new Set(['Onyx']), + methods: new Set(['get', 'multiGet']), + // The rule reports every reference to Onyx.get or Onyx.multiGet that is not a direct call, so a blanket directive there silences it + reportsOnMember: true, + // The rule reports `const api = {...Onyx}` and other value uses of the Onyx object, which hide reads through the copy + reportsOnObjectValue: true, + coversInnerLines: true, + grandfathered: new Map(), + appliesTo: isCheckedSourceFile, + // The module name rather than `Onyx`, because a file with an aliased import such as `import O from 'react-native-onyx'` can read too + searchTerms: [ONYX_MODULE, 'eslint'], + message: + 'An eslint-disable or eslint config comment silences rulesdir/no-unsafe-onyx-read. Onyx.get() and Onyx.multiGet() never subscribe, so a read the rule rejects can leave the UI on stale data. Remove the comment and fix the read: use useOnyx() for data a component renders or reacts to, call Onyx.get() or Onyx.multiGet() only from event handlers in components, pages and hooks that render never calls, and use the Onyx object only through its members, never as a value.', +}; + +const ONYX_UTILS_IMPORT_BAN: BannedRule = { + id: '@typescript-eslint/no-restricted-imports', + name: '@typescript-eslint/no-restricted-imports', + objects: new Set(), + methods: new Set(), + importSources: new Set(['react-native-onyx/dist/OnyxUtils']), + grandfathered: new Map(), + appliesTo: isCheckedSourceFile, + searchTerms: ['OnyxUtils', 'eslint'], + message: + 'An eslint-disable or eslint config comment silences the @typescript-eslint/no-restricted-imports ban on react-native-onyx/dist/OnyxUtils. Reads through OnyxUtils skip no-unsafe-onyx-read and no-onyx-get-snapshot-key. Remove the comment and read with useOnyx(), Onyx.get() or Onyx.multiGet(). Type-only imports of OnyxUtils are still allowed.', +}; + +const BANNED_RULES: BannedRule[] = [ONYX_CONNECT_BAN, ONYX_READ_BAN, ONYX_UTILS_IMPORT_BAN]; + +type SuppressedBan = { + file: string; + line: number; +}; + +type DirectiveMatch = { + index: number; + text: string; + kind?: string; + args: string; +}; + +type BabelComment = { + type: string; + value: string; + start: number | null; + end: number | null; +}; + +// Babel's jsx plugin rejects the angle-bracket assertions (`value`) a `.ts` file may use, so `.ts` files parse without it +function babelPluginsFor(file: string): ParserPlugin[] { + return /\.[cm]?ts$/.test(file) ? ['typescript'] : ['typescript', 'jsx']; +} + +type ParsedSource = {root: ASTNode; comments: BabelComment[]}; + +/** + * Comments found with a regex, for a file Babel cannot parse. The regex does not skip strings, so a `/*` inside a + * string can hide a later directive. The check accepts that gap for a file that is already broken. + */ +function extractCommentsByRegex(source: string): BabelComment[] { + return [...source.matchAll(/\/\*(?[\s\S]*?)\*\/|\/\/(?[^\n\r\u2028\u2029]*)/g)].map((match) => ({ + type: match.groups?.block === undefined ? 'CommentLine' : 'CommentBlock', + value: match.groups?.block ?? match.groups?.line ?? '', + start: match.index, + end: match.index + match[0].length, + })); +} + +/** The Babel AST and comments of `source`, or null when Babel throws or returns something that is not an AST node. */ +function parseSource(source: string, file: string): ParsedSource | null { + try { + const parsed: unknown = parse(source, {sourceType: 'unambiguous', plugins: babelPluginsFor(file), errorRecovery: true, attachComment: true}); + if (!BabelASTUtils.isASTNode(parsed) || !BabelASTUtils.isRecord(parsed)) { + return null; + } + const rawComments = parsed.comments; + const comments = Array.isArray(rawComments) + ? rawComments.filter((comment): comment is BabelComment => { + return ( + BabelASTUtils.isRecord(comment) && + typeof comment.type === 'string' && + typeof comment.value === 'string' && + typeof comment.start === 'number' && + typeof comment.end === 'number' + ); + }) + : []; + return {root: parsed, comments}; + } catch { + return null; + } +} + +// ESLint has -line and -next-line forms only for disable. It ignores `eslint-enable-line` and `eslint-enable-next-line`, +// so reading them as enables would reopen a blanket disable that ESLint still applies. ESLint also needs whitespace or +// the end of the comment after the keyword, so `eslint-enable**` is a plain comment too. +const DIRECTIVE_PATTERNS = { + disable: /^\s*eslint-disable(?-next-line|-line)?(?=\s|$)(?[\s\S]*)$/, + enable: /^\s*eslint-enable(?=\s|$)(?[\s\S]*)$/, + config: /^\s*eslint(?=\s|$)(?[\s\S]*)$/, +}; + +function collectDirectiveMatches(comments: readonly BabelComment[], source: string, directive: keyof typeof DIRECTIVE_PATTERNS): DirectiveMatch[] { + const matches: DirectiveMatch[] = []; + for (const comment of comments) { + if (comment.start === null || comment.end === null) { + continue; + } + const directiveMatch = comment.value.match(DIRECTIVE_PATTERNS[directive]); + if (!directiveMatch) { + continue; + } + // ESLint reads only `-line` and `-next-line` directives from a line comment; `// eslint-enable` and a block-form + // `// eslint-disable` are plain comments to it + if (comment.type === 'CommentLine' && !directiveMatch.groups?.kind) { + continue; + } + matches.push({index: comment.start, text: source.slice(comment.start, comment.end), kind: directiveMatch.groups?.kind, args: directiveMatch.groups?.args ?? ''}); + } + return matches; +} + +const NON_CHILD_KEYS = new Set(['loc', 'start', 'end', 'extra', 'leadingComments', 'trailingComments', 'innerComments', 'comments']); +const WRAPPER_TYPES = new Set(['ParenthesizedExpression', 'TSAsExpression', 'TSSatisfiesExpression', 'TSNonNullExpression', 'TSTypeAssertion']); + +function unwrapExpression(node: ASTNode): ASTNode { + let current = node; + while (WRAPPER_TYPES.has(current.type) && BabelASTUtils.isASTNode(current.expression)) { + current = current.expression; + } + return current; +} + +type BannedNames = { + objects: Set; + methods: Set; +}; + +function identifierName(node: unknown): string | null { + return BabelASTUtils.isASTNode(node) && node.type === 'Identifier' && typeof node.name === 'string' ? node.name : null; +} + +/** The static name of a property key (`get`, `'get'` or a plain template literal), or null for a computed key the lint rules cannot resolve either. */ +function staticKeyName(node: unknown, computed: boolean): string | null { + if (!computed) { + const name = identifierName(node); + if (name !== null) { + return name; + } + } + if (!BabelASTUtils.isASTNode(node)) { + return null; + } + if (node.type === 'StringLiteral' && typeof node.value === 'string') { + return node.value; + } + // cspell:disable-next-line -- quasis is the ESTree name for the static chunks of a template literal + const quasi: unknown = node.type === 'TemplateLiteral' && Array.isArray(node.expressions) && node.expressions.length === 0 && Array.isArray(node.quasis) ? node.quasis.at(0) : null; + const cooked: unknown = BabelASTUtils.isRecord(quasi) && BabelASTUtils.isRecord(quasi.value) ? quasi.value.cooked : null; + return typeof cooked === 'string' ? cooked : null; +} + +function isMemberNode(node: ASTNode): boolean { + return node.type === 'MemberExpression' || node.type === 'OptionalMemberExpression'; +} + +/** True when `node` names the banned object, `Onyx` or an alias of it. Also true for `Onyx.default` on a namespace import, which the rules resolve to the Onyx object too. */ +function isBannedObject(node: unknown, objects: Set): boolean { + if (!BabelASTUtils.isASTNode(node)) { + return false; + } + const current = unwrapExpression(node); + const name = identifierName(current); + if (name !== null) { + return objects.has(name); + } + if (!isMemberNode(current) || staticKeyName(current.property, current.computed === true) !== 'default' || !BabelASTUtils.isASTNode(current.object)) { + return false; + } + const namespaceName = identifierName(unwrapExpression(current.object)); + return namespaceName !== null && objects.has(namespaceName); +} + +function isBannedMember(node: ASTNode, ban: BannedRule, objects: Set): boolean { + if (!isMemberNode(node)) { + return false; + } + const propertyName = staticKeyName(node.property, node.computed === true); + return propertyName !== null && ban.methods.has(propertyName) && isBannedObject(node.object, objects); +} + +/** True for `Onyx[method]`: a member of the banned object whose key no rule can resolve, so it may name a banned method. */ +function isDynamicBannedMember(node: ASTNode, objects: Set): boolean { + return isMemberNode(node) && node.computed === true && staticKeyName(node.property, true) === null && isBannedObject(node.object, objects); +} + +/** + * The names bound to the Onyx object or to a banned method in a file. The lint rules resolve the Onyx import and its + * aliases (`import O from 'react-native-onyx'`, `const api = Onyx`, `const {get} = Onyx`, `const read = Onyx.get`), so a + * blanket directive over an aliased call silences them too. The walk repeats over the declarations until no new name appears. + * + * Matching by name is a deliberate superset of what the rules track: a `let` alias, a parameter that shadows the + * import, or a bare `Onyx` in a file without the import also count. The rules would not report those, but a blanket + * directive over such a call is still a bypass to refuse, and the repo has none today. + */ +function collectBannedNames(root: ASTNode, ban: BannedRule): BannedNames { + const objects = new Set(ban.objects); + const methods = new Set(); + const sizeBefore = () => objects.size + methods.size; + const visit = (node: ASTNode) => { + if (node.type === 'ImportDeclaration' && BabelASTUtils.isASTNode(node.source) && node.source.value === ONYX_MODULE && Array.isArray(node.specifiers)) { + for (const specifier of node.specifiers) { + if (BabelASTUtils.isASTNode(specifier) && (specifier.type === 'ImportDefaultSpecifier' || specifier.type === 'ImportNamespaceSpecifier')) { + const localName = identifierName(specifier.local); + if (localName !== null) { + objects.add(localName); + } + } + } + } + if (node.type === 'VariableDeclarator' && BabelASTUtils.isASTNode(node.id) && BabelASTUtils.isASTNode(node.init)) { + const init = unwrapExpression(node.init); + const idName = identifierName(node.id); + if (idName !== null && isBannedObject(init, objects)) { + objects.add(idName); + } + if (idName !== null && isBannedMember(init, ban, objects)) { + methods.add(idName); + } + if (node.id.type === 'ObjectPattern' && isBannedObject(init, objects) && Array.isArray(node.id.properties)) { + for (const property of node.id.properties) { + if (!BabelASTUtils.isASTNode(property) || property.type !== 'ObjectProperty') { + continue; + } + const keyName = staticKeyName(property.key, property.computed === true); + const valueName = identifierName(property.value); + if (keyName !== null && ban.methods.has(keyName) && valueName !== null) { + methods.add(valueName); + } + } + } + } + for (const child of BabelASTUtils.children(node, NON_CHILD_KEYS)) { + visit(child); + } + }; + let size = -1; + while (size !== sizeBefore()) { + size = sizeBefore(); + visit(root); + } + return {objects, methods}; +} + +function isBannedCall(node: ASTNode, ban: BannedRule, names: BannedNames): boolean { + // Babel parses optional chaining anywhere in a call (`Onyx?.connect(...)`, `Onyx.connect?.(...)`) as + // `OptionalCallExpression` and `OptionalMemberExpression` instead of the plain node types. Both are matched, + // or a blanket disable over an optional call would bypass the ban. + if ((node.type !== 'CallExpression' && node.type !== 'OptionalCallExpression') || !BabelASTUtils.isASTNode(node.callee)) { + return false; + } + const callee = unwrapExpression(node.callee); + const calleeName = identifierName(callee); + if (calleeName !== null) { + return names.methods.has(calleeName); + } + return isBannedMember(callee, ban, names.objects); +} + +type BannedRange = { + start: number; + end: number; +}; + +function bannedDestructuredProperties(node: ASTNode, ban: BannedRule, objects: Set): ASTNode[] { + if ( + node.type !== 'VariableDeclarator' || + !BabelASTUtils.isASTNode(node.id) || + node.id.type !== 'ObjectPattern' || + !BabelASTUtils.isASTNode(node.init) || + !Array.isArray(node.id.properties) + ) { + return []; + } + if (!isBannedObject(node.init, objects)) { + return []; + } + return node.id.properties.filter((property): property is ASTNode => { + if (!BabelASTUtils.isASTNode(property) || property.type !== 'ObjectProperty') { + return false; + } + const keyName = staticKeyName(property.key, property.computed === true); + return keyName !== null && ban.methods.has(keyName); + }); +} + +// Keys under which an identifier names a binding or a label instead of reading the value bound to that name +const BINDING_KEYS = new Set(['id', 'params', 'param', 'label', 'imported', 'exported']); + +const PATTERN_TYPES = new Set(['ObjectPattern', 'ArrayPattern', 'RestElement']); + +const DEV_CONSOLE_GLOBALS = new Set(['window', 'globalThis']); + +const DEV_CONSOLE_PROPERTY = 'Onyx'; + +type AncestorEntry = {node: ASTNode; key: string}; + +/** + * True for `window.Onyx = Onyx`, `window['Onyx'] = Onyx` or `globalThis.Onyx = Onyx as typeof Onyx & {...}`, which expose + * the library to the dev console. Only the Onyx property is exempt, the one react-native-onyx.d.ts declares on Window. + */ +function isDevConsoleExposure(assignment: ASTNode, key: string): boolean { + if (assignment.type !== 'AssignmentExpression' || key !== 'right' || !BabelASTUtils.isASTNode(assignment.left)) { + return false; + } + const target = assignment.left; + if (target.type !== 'MemberExpression' || !BabelASTUtils.isASTNode(target.object) || staticKeyName(target.property, target.computed === true) !== DEV_CONSOLE_PROPERTY) { + return false; + } + const globalName = identifierName(unwrapExpression(target.object)); + return globalName !== null && DEV_CONSOLE_GLOBALS.has(globalName); +} + +/** + * True when the identifier at the end of `ancestors` reads the value it names, outside the member-object position the + * rule allows. Like the rest of this file it goes by name, so a use of a parameter that shadows the import, or of any + * alias in `objects` such as `api` from `const api = Onyx`, counts too, although the rule reports only the import itself. + */ +function isObjectValueUse(ancestors: readonly AncestorEntry[]): boolean { + let index = ancestors.length - 1; + while (index > 0 && ancestors.at(index)?.key === 'expression' && WRAPPER_TYPES.has(ancestors.at(index - 1)?.node.type ?? '')) { + index--; + } + const parent = index > 0 ? ancestors.at(index - 1)?.node : undefined; + const key = ancestors.at(index)?.key ?? ''; + if (!parent || BINDING_KEYS.has(key) || PATTERN_TYPES.has(parent.type) || (key === 'local' && parent.type.startsWith('Import'))) { + return false; + } + if ((parent.type === 'MemberExpression' || parent.type === 'OptionalMemberExpression') && (key === 'object' || parent.computed !== true)) { + return false; + } + if (key === 'key' && parent.computed !== true) { + return false; + } + // A runtime `typeof Onyx` only inspects the object + if (parent.type === 'UnaryExpression' && parent.operator === 'typeof') { + return false; + } + // A property value in a destructuring pattern, or the left side of a default or an assignment, is a binding or a + // write. The rule does report a write such as `Onyx = x`, but that does not typecheck, so it is skipped here + if ((key === 'value' && ancestors.at(index - 2)?.node.type === 'ObjectPattern') || (key === 'left' && (parent.type === 'AssignmentPattern' || parent.type === 'AssignmentExpression'))) { + return false; + } + // `typeof Onyx` and the other type positions read nothing at runtime. A few TS value positions are skipped too + // (`import O = Onyx`, an enum initializer, `Onyx`). Of these, only `import O = Onyx` on a namespace import typechecks + if (parent.type.startsWith('TS') && parent.type !== 'TSExportAssignment') { + return false; + } + return !isDevConsoleExposure(parent, key); +} + +/** + * The nodes the rule reports for a runtime re-export of the Onyx object: each `default` specifier of + * `export {default as Store} from 'react-native-onyx'`, or the whole `export * as Store from 'react-native-onyx'`, which Babel + * parses as a named export with a namespace specifier. + */ +function onyxObjectReExportNodes(node: ASTNode): ASTNode[] { + if (node.type !== 'ExportNamedDeclaration' || importedSource(node) !== ONYX_MODULE || node.exportKind === 'type' || !Array.isArray(node.specifiers)) { + return []; + } + return node.specifiers.flatMap((specifier): ASTNode[] => { + if (!BabelASTUtils.isASTNode(specifier) || specifier.exportKind === 'type') { + return []; + } + if (specifier.type === 'ExportNamespaceSpecifier') { + return [node]; + } + return specifier.type === 'ExportSpecifier' && staticKeyName(specifier.local, false) === 'default' ? [specifier] : []; + }); +} + +/** The start offsets of every value use of the banned object, such as `{...Onyx}`, `fn(Onyx)`, `return Onyx` or a re-export. */ +function collectObjectValueUseRanges(root: ASTNode, objects: Set): BannedRange[] { + const ranges: BannedRange[] = []; + const ancestors: AncestorEntry[] = []; + const visit = (node: ASTNode, key: string) => { + ancestors.push({node, key}); + const name = identifierName(node); + if (name !== null && objects.has(name) && isObjectValueUse(ancestors)) { + ranges.push({start: node.start, end: node.start}); + } + for (const reExport of onyxObjectReExportNodes(node)) { + ranges.push({start: reExport.start, end: reExport.start}); + } + for (const [childKey, value] of Object.entries(node)) { + if (NON_CHILD_KEYS.has(childKey)) { + continue; + } + for (const child of Array.isArray(value) ? value : [value]) { + if (BabelASTUtils.isASTNode(child)) { + visit(child, childKey); + } + } + } + ancestors.pop(); + }; + visit(root, ''); + return ranges; +} + +/** The source ranges a blanket disable must not cover: banned calls, plus the member, destructuring and object value uses that the ban's flags say the rule reports. */ +function collectBannedCallRanges(root: ASTNode, ban: BannedRule, file: string): BannedRange[] { + const names = collectBannedNames(root, ban); + const ranges: BannedRange[] = []; + const visit = (node: ASTNode) => { + if (isBannedCall(node, ban, names)) { + // Unless a rule reports inside the call, the first line alone silences it + ranges.push({start: node.start, end: ban.coversInnerLines ? node.end : node.start}); + } + if (ban.reportsOnMember) { + const memberUses = isBannedMember(node, ban, names.objects) ? [node] : bannedDestructuredProperties(node, ban, names.objects); + for (const use of memberUses) { + ranges.push({start: use.start, end: use.start}); + } + } + if (ban.reportsOnObjectValue && isDynamicBannedMember(node, names.objects)) { + ranges.push({start: node.start, end: node.start}); + } + for (const child of BabelASTUtils.children(node, NON_CHILD_KEYS)) { + visit(child); + } + }; + visit(root); + if (ban.reportsOnObjectValue && !isTestInfrastructureFile(file)) { + ranges.push(...collectObjectValueUseRanges(root, names.objects)); + } + return ranges; +} + +function isTypeOnlyImport(node: ASTNode): boolean { + if (node.importKind === 'type' || node.exportKind === 'type') { + return true; + } + const specifiers = node.specifiers; + return ( + Array.isArray(specifiers) && + specifiers.length > 0 && + specifiers.every((specifier) => BabelASTUtils.isRecord(specifier) && (specifier.importKind === 'type' || specifier.exportKind === 'type')) + ); +} + +function importedSource(node: ASTNode): string | null { + if (node.type === 'ImportDeclaration' || node.type === 'ExportNamedDeclaration' || node.type === 'ExportAllDeclaration') { + return BabelASTUtils.isASTNode(node.source) && typeof node.source.value === 'string' ? node.source.value : null; + } + if (node.type === 'TSImportEqualsDeclaration' && BabelASTUtils.isASTNode(node.moduleReference) && node.moduleReference.type === 'TSExternalModuleReference') { + const expression = node.moduleReference.expression; + return BabelASTUtils.isASTNode(expression) && typeof expression.value === 'string' ? expression.value : null; + } + return null; +} + +function collectBannedImportRanges(root: ASTNode, sources: Set): BannedRange[] { + const ranges: BannedRange[] = []; + const visit = (node: ASTNode) => { + const source = importedSource(node); + if (source !== null && sources.has(source) && !isTypeOnlyImport(node)) { + ranges.push({start: node.start, end: node.start}); + } + for (const child of BabelASTUtils.children(node, NON_CHILD_KEYS)) { + visit(child); + } + }; + visit(root); + return ranges; +} + +/** ESLint splits a directive from its description only on `--` surrounded by whitespace. */ +const DESCRIPTION_SEPARATOR = /\s-{2,}\s/u; + +function directiveRuleList(args: string): string { + return args.split(DESCRIPTION_SEPARATOR).at(0) ?? ''; +} + +function directiveDescription(args: string): string { + const match = DESCRIPTION_SEPARATOR.exec(args); + return match ? args.slice(match.index + match[0].length) : ''; +} + +/** The rule list of a directive, without its description and with runs of whitespace collapsed. A `*` counts as whitespace, since block comment lines start with one. */ +function normalizedDirectiveArgs(args: string): string { + return directiveRuleList(args) + .replaceAll(/[\s*]+/g, ' ') + .trim(); +} + +/** True when the directive's rule list names `ban`, with or without a plugin prefix such as `rulesdir/`. */ +function directiveTargetsBan(args: string, ban: Pick): boolean { + const trimmed = normalizedDirectiveArgs(args); + if (trimmed.length === 0) { + return false; + } + return trimmed.split(',').some((part) => { + // ESLint strips one pair of surrounding quotes from each rule name, so `"rulesdir/x"` means `rulesdir/x` + const rule = part.trim().replace(/^(['"])(.*)\1$/, '$2'); + return rule === ban.id || rule === ban.name || rule.endsWith(`/${ban.name}`); + }); +} + +/** + * True when an `eslint` config comment's rule list mentions `ban.name`, whatever value it sets: `["error", {...}]` can drop + * the OnyxUtils path from the import restriction, so any value is a bypass. A backslash counts too, because ESLint runs + * rule names through JSON.parse, which decodes `\u` escapes. + */ +function configDirectiveNamesBan(args: string, ban: Pick): boolean { + const ruleList = directiveRuleList(args); + return ruleList.includes(ban.name) || ruleList.includes('\\'); +} + +function isBlanketDirective(args: string): boolean { + return normalizedDirectiveArgs(args).length === 0; +} + +/** The 1-based line number at `offset`, counting `\r`, U+2028 and U+2029 as line breaks the way ESLint does. */ +function lineNumberAtOffset(source: string, offset: number): number { + let line = 1; + let index = 0; + const end = Math.min(Math.max(offset, 0), source.length); + while (index < end) { + const code = source.charCodeAt(index); + if (code === 0x0d) { + line++; + index += source.charCodeAt(index + 1) === 0x0a ? 2 : 1; + continue; + } + if (code === 0x0a || code === 0x2028 || code === 0x2029) { + line++; + } + index++; + } + return line; +} + +/** + * True when the directive reaches any line of a banned range. A range spans every line of a call only for a ban with + * `coversInnerLines`, where a line directive inside a multi-line `Onyx.multiGet([...])` still tries to silence the read. + * Every other range is the first line of the use. A block disable stops covering a use once a blanket enable, or one that + * names the ban, comes between them. + */ +function blanketDirectiveCoversCall(source: string, match: DirectiveMatch, bannedRanges: BannedRange[], enableMatches: DirectiveMatch[], ban: BannedRule): boolean { + const directiveLine = lineNumberAtOffset(source, match.index); + const kind = match.kind; + const directiveEnd = match.index + match.text.length; + const directiveEndLine = lineNumberAtOffset(source, Math.max(match.index, directiveEnd - 1)); + return bannedRanges.some((range) => { + const firstLine = lineNumberAtOffset(source, range.start); + const lastLine = lineNumberAtOffset(source, Math.max(range.start, range.end - 1)); + if (kind === '-line') { + return directiveLine >= firstLine && directiveLine <= lastLine; + } + if (kind === '-next-line') { + return directiveEndLine + 1 >= firstLine && directiveEndLine + 1 <= lastLine; + } + if (range.end <= match.index) { + return false; + } + // Only an enable that lands before the call starts frees all of it; one inside the call still leaves lines covered + const reenabled = enableMatches.some((enableMatch) => { + const enableOffset = enableMatch.index; + if (enableOffset <= directiveEnd || enableOffset >= range.start) { + return false; + } + return isBlanketDirective(enableMatch.args) || directiveTargetsBan(enableMatch.args, ban); + }); + return !reenabled; + }); +} + +/** Every directive in `source` that silences `ban`: a disable that names it, a blanket disable over a banned use, or a config comment that names it. Lines are 1-based. */ +function collectDisableDirectivesFromSource(source: string, file: string, ban: BannedRule = ONYX_CONNECT_BAN): SuppressedBan[] { + const parsed = parseSource(source, file); + const bans: SuppressedBan[] = []; + if (!parsed) { + // Fail closed: with no AST to match uses against, every disable directive and config comment counts + const comments = extractCommentsByRegex(source); + for (const match of [...collectDirectiveMatches(comments, source, 'disable'), ...collectDirectiveMatches(comments, source, 'config')]) { + bans.push({file, line: lineNumberAtOffset(source, match.index)}); + } + return bans; + } + const bannedRanges = ban.importSources ? collectBannedImportRanges(parsed.root, ban.importSources) : collectBannedCallRanges(parsed.root, ban, file); + const enableMatches = collectDirectiveMatches(parsed.comments, source, 'enable'); + for (const match of collectDirectiveMatches(parsed.comments, source, 'disable')) { + const args = match.args; + const targetsBan = directiveTargetsBan(args, ban); + const isBlanket = isBlanketDirective(args); + const coversBan = (isBlanket || (!!ban.importSources && targetsBan)) && blanketDirectiveCoversCall(source, match, bannedRanges, enableMatches, ban); + if ((ban.importSources || !targetsBan) && !coversBan) { + continue; + } + bans.push({file, line: lineNumberAtOffset(source, match.index)}); + } + // ESLint applies a config comment to the whole file, so it covers every use without a range check + for (const match of collectDirectiveMatches(parsed.comments, source, 'config')) { + if (configDirectiveNamesBan(match.args, ban) && (!ban.importSources || bannedRanges.length > 0)) { + bans.push({file, line: lineNumberAtOffset(source, match.index)}); + } + } + return bans.sort((a, b) => a.line - b.line); +} + +/** The suppressed bans beyond each file's grandfathered count. The earliest lines in a file use up its allowance first. */ +function findNewBypasses(suppressedBans: readonly SuppressedBan[], rule: BannedRule = ONYX_CONNECT_BAN): SuppressedBan[] { + const byFile = new Map(); + for (const ban of suppressedBans) { + const list = byFile.get(ban.file) ?? []; + list.push(ban); + byFile.set(ban.file, list); + } + + const newBypasses: SuppressedBan[] = []; + for (const [file, bans] of byFile) { + const allowed = rule.grandfathered.get(file) ?? 0; + if (bans.length <= allowed) { + continue; + } + const sortedByLine = [...bans].sort((a, b) => a.line - b.line); + newBypasses.push(...sortedByLine.slice(allowed)); + } + return newBypasses; +} + +/** + * Rules the seatbelt ratchet must never grandfather. `SEATBELT_INCREASE=` writes a row that turns the rule's + * errors in that file into allowed ones, which silences the rule as fully as a disable directive. + * `rulesdir/no-onyx-connect` is not listed, because its existing rows are the backlog still waiting for migration. + */ +const SEATBELT_FORBIDDEN_RULES: readonly string[] = ['rulesdir/no-unsafe-onyx-read', 'rulesdir/no-onyx-get-snapshot-key']; + +/** A seatbelt row for a rule the seatbelt may not cover. `file` is as written in the TSV, relative to its directory. */ +type ForbiddenSeatbeltRow = { + file: string; + rule: string; + line: number; +}; + +/** A TSV cell, decoded the way the seatbelt reads it (as JSON), so an escape such as `\u002f` cannot hide a rule id. */ +function decodeSeatbeltCell(cell: string): string { + try { + const value: unknown = JSON.parse(cell); + if (typeof value === 'string') { + return value; + } + } catch { + // Not JSON: fall back to stripping one pair of surrounding quotes + } + return cell.trim().replace(/^"(.*)"$/, '$1'); +} + +function findForbiddenSeatbeltRows(tsvText: string, forbiddenRules: readonly string[]): ForbiddenSeatbeltRow[] { + const rows: ForbiddenSeatbeltRow[] = []; + for (const [index, rawLine] of tsvText.split(/\r?\n/).entries()) { + if (rawLine.trim().length === 0 || /^\s*#/.test(rawLine)) { + continue; + } + const [file = '', rule = ''] = rawLine.split('\t').map(decodeSeatbeltCell); + if (forbiddenRules.includes(rule)) { + rows.push({file, rule, line: index + 1}); + } + } + return rows; +} + +export { + BANNED_RULE_ID, + BANNED_RULES, + ONYX_READ_BAN, + ONYX_UTILS_IMPORT_BAN, + SEATBELT_FORBIDDEN_RULES, + collectDirectiveMatches, + collectDisableDirectivesFromSource, + findForbiddenSeatbeltRows, + configDirectiveNamesBan, + directiveTargetsBan, + findNewBypasses, + directiveDescription, + extractCommentsByRegex, + isCheckedSourceFile, + isLintedFile, + lineNumberAtOffset, + parseSource, +}; +export type {BannedRule, ForbiddenSeatbeltRow}; diff --git a/scripts/onyxLintDisableReason.ts b/scripts/onyxLintDisableReason.ts new file mode 100644 index 000000000000..4219474f6f81 --- /dev/null +++ b/scripts/onyxLintDisableReason.ts @@ -0,0 +1,81 @@ +/** + * Finds inline `eslint-disable` directives of the rules in `REASONED_RULES` that give no reason. + * Unlike the bans in `onyxLintBypass.ts`, these rules may be disabled, but only one line at a time + * and only with a reason after `--`. The reason then sits next to the exception, where a reviewer + * can judge it. + * + * A directive counts when it names the rule, in the `-line` or `-next-line` form, and its text after + * `--` holds fewer than `MIN_REASON_LENGTH` letters or digits. A block `eslint-disable` that names the + * rule always counts, even with a reason: one sentence cannot vouch for every read under it. Blanket + * directives that name no rule are left to `onyxLintBypass.ts`. A file Babel cannot parse fails closed: + * every disable naming the rule counts. An `eslint` config comment naming the rule always counts. + */ + +import { + collectDirectiveMatches, + configDirectiveNamesBan, + directiveDescription, + directiveTargetsBan, + extractCommentsByRegex, + isCheckedSourceFile, + lineNumberAtOffset, + parseSource, +} from './onyxLintBypass'; + +/** Fewest letters and digits after `--` that count as a reason, so `-- ok`, `-- todo` or a row of dashes does not pass. */ +const MIN_REASON_LENGTH = 12; + +type ReasonedRule = { + id: string; + name: string; + appliesTo: (file: string) => boolean; + searchTerms: string[]; + message: string; +}; + +const ONYX_SNAPSHOT_KEY_RULE: ReasonedRule = { + id: 'rulesdir/no-onyx-get-snapshot-key', + name: 'no-onyx-get-snapshot-key', + appliesTo: isCheckedSourceFile, + searchTerms: ['no-onyx-get-snapshot-key', 'eslint'], + message: `A disable of rulesdir/no-onyx-get-snapshot-key gives no reason, or is a block eslint-disable or an eslint config comment. Each exception must say, on the line it covers, why live data is right for this Search snapshot key, so a reviewer can judge it. Use eslint-disable-next-line or eslint-disable-line with a reason of at least ${MIN_REASON_LENGTH} letters or digits after \`--\`, for example: \`// eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- replaces a module-level Onyx.connect used only by handlers, never read a snapshot\`.`, +}; + +const REASONED_RULES: ReasonedRule[] = [ONYX_SNAPSHOT_KEY_RULE]; + +type UnreasonedDisable = { + file: string; + line: number; +}; + +function hasReason(args: string): boolean { + return (directiveDescription(args).match(/[\p{L}\p{N}]/gu) ?? []).length >= MIN_REASON_LENGTH; +} + +/** Every disable of `rule` in `source` that lacks an acceptable reason. Lines are 1-based. */ +function collectUnreasonedDisables(source: string, filePath: string, rule: ReasonedRule = ONYX_SNAPSHOT_KEY_RULE): UnreasonedDisable[] { + const parsed = parseSource(source, filePath); + // Fail closed: when Babel cannot parse the file, comments come from a regex and every directive naming the rule counts + const comments = parsed ? parsed.comments : extractCommentsByRegex(source); + const unreasoned: UnreasonedDisable[] = []; + for (const match of collectDirectiveMatches(comments, source, 'disable')) { + if (!directiveTargetsBan(match.args, rule)) { + continue; + } + const isLineForm = match.kind === '-line' || match.kind === '-next-line'; + if (parsed && isLineForm && hasReason(match.args)) { + continue; + } + unreasoned.push({file: filePath, line: lineNumberAtOffset(source, match.index)}); + } + // A config comment turns the rule off for the whole file, so no reason can cover it + for (const match of collectDirectiveMatches(comments, source, 'config')) { + if (configDirectiveNamesBan(match.args, rule)) { + unreasoned.push({file: filePath, line: lineNumberAtOffset(source, match.index)}); + } + } + return unreasoned.sort((a, b) => a.line - b.line); +} + +export {MIN_REASON_LENGTH, ONYX_SNAPSHOT_KEY_RULE, REASONED_RULES, collectUnreasonedDisables}; +export type {ReasonedRule}; diff --git a/src/components/Search/SearchResultsProvider.tsx b/src/components/Search/SearchResultsProvider.tsx index 9691456eec21..8bb9bfb17897 100644 --- a/src/components/Search/SearchResultsProvider.tsx +++ b/src/components/Search/SearchResultsProvider.tsx @@ -11,7 +11,7 @@ import type {SearchResultsInfo} from '@src/types/onyx/SearchResults'; import React, {useState} from 'react'; // This provider is the source of the snapshot data that `@hooks/useOnyx` later routes consumers onto, // so going through that wrapper here would be self-referential. The wrapper also short-circuits its own -// logic for snapshot keys (see the `!key.startsWith(ONYXKEYS.COLLECTION.SNAPSHOT)` guard in useOnyx.ts), +// logic for snapshot keys (see the snapshot collection guard in `isSnapshotCompatibleKey` in @libs/SnapshotUtils), // so it would add nothing for this read. Use the raw react-native-onyx hook directly. // eslint-disable-next-line no-restricted-imports import {useOnyx} from 'react-native-onyx'; diff --git a/src/hooks/useOnyx.ts b/src/hooks/useOnyx.ts index 323584c1c069..b078cc80726b 100644 --- a/src/hooks/useOnyx.ts +++ b/src/hooks/useOnyx.ts @@ -1,6 +1,8 @@ import {SearchQueryContext, SearchResultsContext} from '@components/Search/SearchContext'; import {useIsOnSearch} from '@components/Search/SearchScopeProvider'; +import isSnapshotCompatibleKey from '@libs/SnapshotUtils'; + import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {SearchResults} from '@src/types/onyx'; @@ -73,12 +75,12 @@ function resolveSnapshotAwareResult( * Custom hook for accessing and subscribing to Onyx data with search snapshot support */ const useOnyx: UseOnyxWithoutSnapshots = >(key: TKey, options?: UseOnyxOptions) => { - const isSnapshotCompatibleKey = !key.startsWith(ONYXKEYS.COLLECTION.SNAPSHOT) && CONST.SEARCH.SNAPSHOT_ONYX_KEYS.some((snapshotKey) => key.startsWith(snapshotKey)); + const isSnapshotCompatible = isSnapshotCompatibleKey(key, CONST.SEARCH.SNAPSHOT_ONYX_KEYS); const isOnSearch = useIsOnSearch(); let currentSearchHash: number | undefined; let shouldUseLiveData = false; - if (isOnSearch && isSnapshotCompatibleKey) { + if (isOnSearch && isSnapshotCompatible) { const {currentSearchHash: searchContextCurrentSearchHash} = use(SearchQueryContext); const {shouldUseLiveData: contextShouldUseLiveData} = use(SearchResultsContext); currentSearchHash = searchContextCurrentSearchHash; @@ -89,7 +91,7 @@ const useOnyx: UseOnyxWithoutSnapshots = | undefined) => selectorProp(getKeyData(data as SearchResults, key)); diff --git a/src/libs/SnapshotUtils.ts b/src/libs/SnapshotUtils.ts new file mode 100644 index 000000000000..27f3568f82db --- /dev/null +++ b/src/libs/SnapshotUtils.ts @@ -0,0 +1,12 @@ +import type ONYXKEYS from '@src/ONYXKEYS'; + +// A literal rather than ONYXKEYS.COLLECTION.SNAPSHOT: the no-onyx-get-snapshot-key lint rule loads this file under plain node, where +// neither the @src alias nor extension-less imports resolve. The satisfies clause pins it to that key. +const SNAPSHOT_COLLECTION_KEY = 'snapshot_' satisfies (typeof ONYXKEYS)['COLLECTION']['SNAPSHOT']; + +/** True when `useOnyx` would redirect `key` to a Search snapshot. A `snapshot_` key itself never counts, whatever `snapshotKeys` holds. */ +function isSnapshotCompatibleKey(key: string, snapshotKeys: readonly string[]): boolean { + return !key.startsWith(SNAPSHOT_COLLECTION_KEY) && snapshotKeys.some((snapshotKey) => key.startsWith(snapshotKey)); +} + +export default isSnapshotCompatibleKey; diff --git a/src/setup/addUtilsToWindow.ts b/src/setup/addUtilsToWindow.ts index ec3935f4e2b3..e1147d5cd66b 100644 --- a/src/setup/addUtilsToWindow.ts +++ b/src/setup/addUtilsToWindow.ts @@ -24,24 +24,9 @@ export default function addUtilsToWindow() { } window.Onyx = Onyx as typeof Onyx & { - get: (key: CollectionKeyBase) => Promise; log: (key: CollectionKeyBase) => void; }; - // We intentionally do not offer an Onyx.get API because we believe it will lead to code patterns we don't want to use in this repo, but we can offer a workaround for the sake of debugging - window.Onyx.get = function (key: CollectionKeyBase) { - return new Promise((resolve) => { - // We have opted for `connectWithoutView` here as this is a debugging utility and does not relate to any view. - const connection = Onyx.connectWithoutView({ - key, - callback: (value) => { - Onyx.disconnect(connection); - resolve(value); - }, - }); - }); - }; - window.Onyx.log = function (key: CollectionKeyBase) { window.Onyx.get(key).then((value) => { /* eslint-disable-next-line no-console */ diff --git a/src/types/modules/react-native-onyx.d.ts b/src/types/modules/react-native-onyx.d.ts index 73615460afd9..c091f53bce96 100644 --- a/src/types/modules/react-native-onyx.d.ts +++ b/src/types/modules/react-native-onyx.d.ts @@ -16,7 +16,6 @@ declare global { // eslint-disable-next-line @typescript-eslint/consistent-type-definitions interface Window { Onyx: typeof Onyx & { - get: (key: CollectionKeyBase) => Promise; log: (key: CollectionKeyBase) => void; }; } diff --git a/tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts b/tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts new file mode 100644 index 000000000000..aa9d12496ad6 --- /dev/null +++ b/tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts @@ -0,0 +1,473 @@ +/** + * @jest-environment node + */ +import isSnapshotCompatibleKey from '@libs/SnapshotUtils'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; + +import type {Rule} from 'eslint'; + +import {Linter, RuleTester} from 'eslint'; +import {parser as tsParser} from 'typescript-eslint'; + +type LocalRuleModule = Rule.RuleModule & { + name: string; +}; + +function isLocalRuleModule(ruleModule: unknown): ruleModule is LocalRuleModule { + if (typeof ruleModule !== 'object' || ruleModule === null) { + return false; + } + + const ruleName: unknown = Reflect.get(ruleModule, 'name'); + const create: unknown = Reflect.get(ruleModule, 'create'); + const meta: unknown = Reflect.get(ruleModule, 'meta'); + + return typeof ruleName === 'string' && typeof create === 'function' && typeof meta === 'object' && meta !== null; +} + +function loadLocalRule(ruleName: string): LocalRuleModule { + const ruleModule: unknown = require(`../../eslint-plugin-local-rules/${ruleName}`); + + if (!isLocalRuleModule(ruleModule)) { + throw new TypeError(`Expected ${ruleName} to export an ESLint rule module.`); + } + + return ruleModule; +} + +const ruleModule = loadLocalRule('no-onyx-get-snapshot-key'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'module', + parserOptions: { + ecmaFeatures: {jsx: true}, + }, + }, +}); + +const tsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'module', + parser: tsParser, + parserOptions: { + ecmaFeatures: {jsx: true}, + }, + }, +}); + +const ONYX_IMPORT = "import Onyx from 'react-native-onyx';"; +const USE_ONYX_IMPORT = "import useOnyx from '@hooks/useOnyx';"; + +const RESTRICTED_ERRORS = [{messageId: 'noRestrictedOnyxKey'}]; + +const UNRESOLVABLE_ERRORS = [{messageId: 'noUnresolvableOnyxKey'}]; + +describe('no-onyx-get-snapshot-key restricted keys', () => { + ruleTester.run(ruleModule.name, ruleModule, { + valid: [ + {code: `${ONYX_IMPORT} export function submit() { return Onyx.get(ONYXKEYS.SESSION); }`}, + + // POLICY_CATEGORIES is not one of the Search snapshot collections, and a const key resolves to what it holds + {code: `${ONYX_IMPORT} export function submit(id) { return Onyx.get(\`\${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}\${id}\`); }`}, + {code: `${ONYX_IMPORT} export function submit() { const key = ONYXKEYS.SESSION; return Onyx.get(key); }`}, + + // The position rule owns where a read may run, so this rule reports nothing about render or module scope + {code: `${ONYX_IMPORT} function Row() { const value = Onyx.get(ONYXKEYS.SESSION); return ; }`}, + {code: `${ONYX_IMPORT} const initialValue = Onyx.get(ONYXKEYS.SESSION);`}, + + // A same-named object that is not the Onyx import is not tracked + {code: 'const Onyx = {get: () => undefined}; export function submit() { return Onyx.get(ONYXKEYS.COLLECTION.REPORT); }'}, + {code: `${ONYX_IMPORT} export function submit() { return window.Onyx.get(ONYXKEYS.COLLECTION.REPORT); }`}, + ], + invalid: [ + {code: `${ONYX_IMPORT} export function submit() { return Onyx.get(ONYXKEYS.COLLECTION.REPORT); }`, errors: RESTRICTED_ERRORS}, + { + code: `${ONYX_IMPORT} export function submit(reportID) { return Onyx.get(\`\${ONYXKEYS.COLLECTION.REPORT}\${reportID}\`); }`, + errors: RESTRICTED_ERRORS, + }, + + { + code: `${ONYX_IMPORT} export function submit() { const key = ONYXKEYS.COLLECTION.REPORT; return Onyx.get(key); }`, + errors: RESTRICTED_ERRORS, + }, + { + code: `${ONYX_IMPORT} export function submit(reportID) { const key = \`\${ONYXKEYS.COLLECTION.REPORT}\${reportID}\`; return Onyx.get(key); }`, + errors: RESTRICTED_ERRORS, + }, + {code: `${ONYX_IMPORT} export function submit(key) { return Onyx.get(key); }`, errors: UNRESOLVABLE_ERRORS}, + {code: `${ONYX_IMPORT} export function submit() { return Onyx.get(ONYXKEYS.COLLECTION); }`, errors: UNRESOLVABLE_ERRORS}, + {code: `${ONYX_IMPORT} export function submit() { let key = ONYXKEYS.SESSION; key = other; return Onyx.get(key); }`, errors: UNRESOLVABLE_ERRORS}, + {code: `${ONYX_IMPORT} export function submit(id) { return Onyx.get(getTravelCardKey(id)); }`, errors: UNRESOLVABLE_ERRORS}, + { + code: `${ONYX_IMPORT} export function submit(formID) { return Onyx.get(\`\${formID}Draft\`); }`, + errors: UNRESOLVABLE_ERRORS, + }, + // A computed member or a static prefix before the ONYXKEYS access cannot be resolved to one key + {code: `${ONYX_IMPORT} export function submit(name) { return Onyx.get(ONYXKEYS.COLLECTION[name]); }`, errors: UNRESOLVABLE_ERRORS}, + {code: `${ONYX_IMPORT} export function submit() { return Onyx.get(\`report_\${ONYXKEYS.ACCOUNT}\`); }`, errors: UNRESOLVABLE_ERRORS}, + // Aliases resolve by binding, not by source order, and a let alias still counts as a read + {code: `${ONYX_IMPORT} export function submit() { return read(ONYXKEYS.COLLECTION.REPORT); } const read = Onyx.get;`, errors: RESTRICTED_ERRORS}, + {code: `${ONYX_IMPORT} let read = Onyx.get; export function submit() { return read(ONYXKEYS.COLLECTION.REPORT); }`, errors: RESTRICTED_ERRORS}, + + // The key check does not depend on the position of the read + {code: `${ONYX_IMPORT} function Row() { const value = Onyx.get(ONYXKEYS.COLLECTION.REPORT); return ; }`, errors: RESTRICTED_ERRORS}, + {code: `${ONYX_IMPORT} const initialValue = Onyx.get(ONYXKEYS.PERSONAL_DETAILS_LIST);`, errors: RESTRICTED_ERRORS}, + + // A local object named ONYXKEYS is not the real key map, so the key cannot be resolved + {code: `${ONYX_IMPORT} const ONYXKEYS = {SESSION: 'report_1'}; export function submit() { return Onyx.get(ONYXKEYS.SESSION); }`, errors: UNRESOLVABLE_ERRORS}, + {code: `${ONYX_IMPORT} import ONYXKEYS from '@src/ONYXKEYS'; export function submit() { return Onyx.get(ONYXKEYS.COLLECTION.REPORT); }`, errors: RESTRICTED_ERRORS}, + {code: `${ONYX_IMPORT} import ONYXKEYS from './myKeys'; export function submit() { return Onyx.get(ONYXKEYS.SESSION); }`, errors: UNRESOLVABLE_ERRORS}, + + // Aliases of the read methods are tracked the same way + {code: `${ONYX_IMPORT} const {get} = Onyx; export function submit() { return get(ONYXKEYS.COLLECTION.REPORT); }`, errors: RESTRICTED_ERRORS}, + {code: `${ONYX_IMPORT} const readOnyx = Onyx.get; export function submit() { return readOnyx(ONYXKEYS.COLLECTION.REPORT); }`, errors: RESTRICTED_ERRORS}, + {code: `${ONYX_IMPORT} const api = Onyx; export function submit() { return api.get(ONYXKEYS.COLLECTION.REPORT); }`, errors: RESTRICTED_ERRORS}, + ], + }); +}); + +describe('no-onyx-get-snapshot-key Concierge chat key', () => { + ruleTester.run(ruleModule.name, ruleModule, { + valid: [ + // The report ID is a const read of ONYXKEYS.CONCIERGE_REPORT_ID in the same file + { + code: `${ONYX_IMPORT} export async function submit() { const conciergeReportID = await Onyx.get(ONYXKEYS.CONCIERGE_REPORT_ID); return Onyx.get(\`\${ONYXKEYS.COLLECTION.REPORT}\${conciergeReportID}\`); }`, + }, + { + code: `${ONYX_IMPORT} ${USE_ONYX_IMPORT} function Row() { const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID); const onPress = async () => Onyx.get(\`\${ONYXKEYS.COLLECTION.REPORT}\${conciergeReportID}\`); return