diff --git a/.claude/skills/app-coding-standards/SKILL.md b/.claude/skills/app-coding-standards/SKILL.md index 376bac8bd283..94054f1cb89b 100644 --- a/.claude/skills/app-coding-standards/SKILL.md +++ b/.claude/skills/app-coding-standards/SKILL.md @@ -76,6 +76,9 @@ Coding standards for the Expensify App. Each standard is a standalone file in `r - [UI-3](rules/ui-3-no-inline-styles.md) — Do not use inline style objects - [UI-4](rules/ui-4-layout-spacing-tokens.md) — Type and responsive insets come from tokens +### 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..d44fca3e967c --- /dev/null +++ b/.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md @@ -0,0 +1,215 @@ +--- +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` and `no-onyx-get-snapshot-key` catch 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 during render or at module scope | `no-unsafe-onyx-read` | +| A read inside an effect, or in a same-file function an effect calls | `no-unsafe-onyx-read` | +| A Search snapshot key, or a key lint can't resolve, including any `multiGet` element | `no-onyx-get-snapshot-key` | +| The `useSnapshotOnyxGet()` reader called with a non-snapshot key, a key lint can't resolve, or the Concierge chat | `no-onyx-get-snapshot-key` | +| The `useSnapshotOnyxGet()` reader passed to another component or function, returned, or stored anywhere but a local variable | `no-unsafe-onyx-read` | +| A runtime import of `react-native-onyx/dist/OnyxUtils` | `@typescript-eslint/no-restricted-imports` | +| An inline `eslint-disable` of `no-unsafe-onyx-read`, one over a runtime OnyxUtils import, or a `no-onyx-get-snapshot-key` disable without a reason after `--` | `scripts/checkOnyxConnectBypass.ts` | +| A missing `await` whose value is then used | `tsc` | + +What's left crosses a file boundary, depends on write ordering, only shows in the diff, or happens after the read. + +Mutating a read result writes the cache, since the value is the cached object. `useOnyx` hands out the same object, so this stays a documented convention and isn't flagged here. + +**A. Position.** Render reaches a read wherever it's written. Lint counts a function as a render body only when it's named like a component or hook or has a top-level `return `, plus `selector` options, lazy initializers and `useSyncExternalStore` snapshots. It misses a helper that a hook calls from render, a helper that returns JSX from inside an `if` or `switch`, and a function passed to a child that calls it during render. + +**B. Tick.** Awaiting a read doesn't wait for an earlier write. `Onyx.get` captures the cached value when it's called, and `merge` and `update` apply to the cache later, so a read queued behind them sees the old value. `set` lands at once today, but don't rely on it: 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. + +**E. Live read of a snapshot key.** A `no-onyx-get-snapshot-key` disable claims the code wants live data. That holds only when the value it replaces was live too: `useOnyxWithoutSnapshots`, `Onyx.connect`, or a `useOnyx` call that runs outside every `SearchScopeProvider`, such as one in a provider mounted above the Search list. A `useOnyx` read of the same key in a component inside a Search scope showed the snapshot, so reading it live changes what the code acts on. Read it with the reader `useSnapshotOnyxGet()` returns instead, which resolves the same snapshot. + +### Incorrect + +**A1. A hook calls a reading helper from render.** + +```ts +// src/hooks/useCurrentUserEmail.ts +async function getCurrentUserEmail() { + return (await Onyx.get(ONYXKEYS.SESSION))?.email; // fine on its own +} + +function useCurrentUserEmail() { + return getCurrentUserEmail(); // render gets a Promise, and tsc accepts it +} +``` + +**A2. A render body lint doesn't recognize.** + +```tsx +// Every return is JSX, but inside a switch, so lint sees 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 ; + } +} + + renderTagBadge(item.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}; +} +``` + +**E. A snapshot read turned live.** + +```tsx +// Inside a Search list row, which renders under SearchScopeProvider +function Row({reportID}: Props) { + // Removed: const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); which read the snapshot + const onPress = async () => { + // eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- needs the latest report + approve(await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`)); + }; +} +``` + +### 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}; + +// E: the reader keeps the snapshot a Search row showed. +const getOnyx = useSnapshotOnyxGet(); +const onPress = async () => approve(await getOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`)); + +// E: a disable only where the replaced read was already live. +// Removed: const [report] = useOnyxWithoutSnapshots(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); +const onPress = async () => { + // eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- replaces a useOnyxWithoutSnapshots read, so the row already acted on live data + approve(await Onyx.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`)); +}; +``` + +--- + +### 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. A plain-function caller isn't a verdict, so repeat on its name. Comment on the read, naming the calling file and line. +- 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. A write that isn't awaited is followed in the same tick by a call whose file reads the written key, a member of the written collection, or a `DERIVED` key built from it (see `dependencies` in `src/libs/actions/OnyxDerived/configs/`). 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. 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. Live read of a snapshot key + +- E1. The diff adds a `no-onyx-get-snapshot-key` disable. Find what the read replaces: a removed `useOnyx` line in the diff, or the hook or context the value came from before. Flag the disable when that was the `@hooks/useOnyx` wrapper reading the same key in a component that renders under a `SearchScopeProvider` (rows under `src/components/Search/`, or anything mounted inside the Search list). Comment on the disable, naming the replaced subscription and suggesting the `useSnapshotOnyxGet()` reader. + +**DO NOT flag if:** + +- The read sits in an event handler or `useCallback` body that render doesn't invoke, and the reading function isn't exported, isn't a render body by A2, and isn't passed as a render callback +- The prop holding the reader is named `on*` or `handle*`, and every receiver attaches it to an event or calls it from a handler +- 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 snapshot of 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 +- (E only) The disabled read replaces `useOnyxWithoutSnapshots`, `Onyx.connect`, or a `useOnyx` call in a provider or screen mounted outside `SearchScopeProvider` + +**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 +- `no-onyx-get-snapshot-key` in added `eslint-disable` comments diff --git a/.github/workflows/onyxGetReviewers.yml b/.github/workflows/onyxGetReviewers.yml new file mode 100644 index 000000000000..952395b560cd --- /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, since docs, lint rules + // and scripts mention `Onyx.get(` in prose and messages. Jest mocks are excluded, and we + // compare added vs. removed call lines per file (a call matches `Onyx.get(` or `Onyx.multiGet(`, + // but not `Onyx.getAllKeys(` or the dev-console `window.Onyx.get(`), so editing a call in place + // nets to zero, lines that start with a comment don't count on either side, and a removal in one + // file can't mask a new call in another. + 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/config/eslint/eslint.config.mjs b/config/eslint/eslint.config.mjs index 9b6129fdbc7d..edc2aed061a0 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/ONYX-DATA-MANAGEMENT.md b/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md index 60c7fb78c89d..099b983beaf6 100644 --- a/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md +++ b/contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md @@ -37,10 +37,12 @@ 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. @@ -60,6 +62,23 @@ 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 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`, and `Onyx.get()` always reads the global key. Inside an event handler, read these keys with the reader `useSnapshotOnyxGet()` returns, which resolves the snapshot the same way. No element of an `Onyx.multiGet()` key list may be one of them; write that list as an array literal of static keys, or a `const` bound to one, so lint can check each element. When the code deliberately wants live data (it replaces `useOnyxWithoutSnapshots` or `Onyx.connect`), disable `rulesdir/no-onyx-get-snapshot-key` on that line with a reason after `--`. The Concierge chat is never in a snapshot, so `Onyx.get()` may read a report key built from `ONYXKEYS.CONCIERGE_REPORT_ID` without a disable. + +### - Reads MUST come before a write in the same tick, or after the write is awaited +`Onyx.get()` captures the cache when it is called, and most writes land later, so a read queued behind a write 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, not the `selector` projection `useOnyx` hands out, and a single-key read is the cached object itself, so writing to it changes the cache without telling subscribers. + +`rulesdir/no-unsafe-onyx-read` enforces where a read may happen and cannot be disabled inline. `rulesdir/no-onyx-get-snapshot-key` enforces which keys it may read, and may be disabled only with a reason after `--`. [ONYX-1](../../.claude/skills/app-coding-standards/rules/onyx-1-no-render-reachable-onyx-read.md) covers the rest in review, with 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..9a4f1c485a98 --- /dev/null +++ b/eslint-plugin-local-rules/no-onyx-get-snapshot-key.js @@ -0,0 +1,224 @@ +// Node ESM needs the extension to load a rule's helper module +/* eslint-disable import/extensions */ +import { + createOnyxReadTracker, + getConstInitializer, + getKeyListElements, + getOnyxKeyPath, + getRepoRelativePath, + getVariableByName, + matchesCalleeName, + MULTI_READ_METHOD, + ONYXKEYS_ROOT, + READ_METHOD, + RESTRICTED_KEY_PATHS, + SNAPSHOT_READ_METHOD, + TYPE_ONLY_EXPRESSIONS, +} from './utils/onyxReadUtils.js'; +/* eslint-enable import/extensions */ + +const name = 'no-onyx-get-snapshot-key'; + +// useSnapshotOnyxGet resolves snapshot keys itself, so its own reads skip the key checks +const SNAPSHOT_AWARE_READ_FILES = new Set(['src/hooks/useSnapshotOnyxGet.ts']); + +// The Concierge chat is never part of a Search snapshot, so a report key provably built from this ID is always read live +const CONCIERGE_REPORT_ID_KEY_PATH = 'CONCIERGE_REPORT_ID'; + +const CONCIERGE_CHAT_COLLECTION_KEY_PATH = 'COLLECTION.REPORT'; + +const ONYX_ID_NORMALIZER_NAMES = new Set(['getNonEmptyStringOnyxID']); + +const USE_ONYX_HOOK_NAMES = new Set(['useOnyx', 'useOnyxWithoutSnapshots']); + +const meta = { + type: 'problem', + docs: { + description: + 'Disallow Onyx.get and Onyx.multiGet on Search snapshot keys and on keys the rule cannot resolve, and the useSnapshotOnyxGet() reader on any other key. Disable it only with a reason after `--`.', + recommended: 'error', + }, + schema: [], + messages: { + noRestrictedOnyxKey: + 'Do not read {{keyPath}} with a one-shot Onyx read. src/hooks/useOnyx.ts rewrites this key to snapshot_ inside a SearchScopeProvider subtree, so a component subscribed to it may never have been reading the global key at all. A read here returns live data where the component saw the snapshot, and nothing at the call site can tell the two apart.\n\n' + + 'In an event handler, use useSnapshotOnyxGet() from src/hooks/useSnapshotOnyxGet.ts: call `const getOnyx = useSnapshotOnyxGet();` in the component and `await getOnyx(key)` in the handler. It reads the same snapshot useOnyx would and does not subscribe. Keep useOnyx for values the component renders.\n\n' + + 'If live data is right on purpose (the code replaces useOnyxWithoutSnapshots or Onyx.connect), disable this rule on the line with a reason: `// eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- `.', + noUnresolvableOnyxKey: + 'Do not read Onyx with a key this rule cannot resolve. A key built at runtime cannot be checked against the Search snapshot keys, so a caller can route a snapshot key here without anything failing.\n\n' + + '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, whose every element is written that way. If the key cannot be static, disable this rule on the line with a reason after `--`.', + noNonSnapshotKeyInSnapshotReader: + 'Do not read {{keyPath}} with the useSnapshotOnyxGet() reader. The reader is for Search snapshot keys, which useOnyx may read from snapshot_, and {{keyPath}} is never read from a snapshot.\n\n' + + 'Use Onyx.get() for it.', + noUnresolvableSnapshotReaderKey: + 'Do not call the useSnapshotOnyxGet() reader with a key this rule cannot resolve. The reader only accepts Search snapshot keys, and this rule cannot check a key built at runtime.\n\n' + + 'Write the key as an ONYXKEYS access, such as ONYXKEYS.PERSONAL_DETAILS_LIST, or as a template literal that starts with an ONYXKEYS collection prefix, such as ONYXKEYS.COLLECTION.REPORT followed by the report ID.', + noConciergeChatInSnapshotReader: + 'Do not read the Concierge chat with the useSnapshotOnyxGet() reader. Search snapshots never include it, so inside a SearchScopeProvider the reader returns undefined.\n\n' + + 'Read it with Onyx.get(), which this rule allows for a report key built from ONYXKEYS.CONCIERGE_REPORT_ID.', + }, +}; + +function findRestrictedKey(keyArgument, scope) { + const keyPath = getOnyxKeyPath(keyArgument, scope); + + if (!keyPath) { + return {keyPath: null}; + } + + return RESTRICTED_KEY_PATHS.has(keyPath) ? {keyPath} : null; +} + +function findRestrictedKeys(readMethod, call, scope) { + const keyArgument = call.arguments.at(0); + + if (readMethod !== MULTI_READ_METHOD) { + const finding = findRestrictedKey(keyArgument, scope); + + return finding ? [{node: call, ...finding}] : []; + } + + const elements = keyArgument?.type === 'SpreadElement' ? null : getKeyListElements(keyArgument, scope); + + if (!elements) { + return [{node: call, keyPath: null}]; + } + + return elements.flatMap((element) => { + if (!element || element.type === 'SpreadElement') { + return [{node: element ?? call, keyPath: null}]; + } + + const finding = findRestrictedKey(element, scope); + + return finding ? [{node: element, ...finding}] : []; + }); +} + +function create(context) { + const sourceCode = context.sourceCode ?? context.getSourceCode(); + const filename = context.filename ?? context.getFilename(); + const {visitors, getCalledReadMethod} = createOnyxReadTracker(sourceCode); + + function isOnyxReadOf(node, keyPath, scope) { + const call = node?.type === 'AwaitExpression' ? node.argument : node; + + if (call?.type !== 'CallExpression') { + return false; + } + + const isOnyxGet = getCalledReadMethod(call.callee, scope) === READ_METHOD; + const isUseOnyx = matchesCalleeName(call.callee, USE_ONYX_HOOK_NAMES); + + return (isOnyxGet || isUseOnyx) && getOnyxKeyPath(call.arguments.at(0), scope) === keyPath; + } + + // True when the ID is a const read straight from ONYXKEYS.CONCIERGE_REPORT_ID, by Onyx.get or by a useOnyx tuple + function isConciergeReportIDExpression(node, scope) { + let current = node; + + while (current?.type === 'CallExpression' && matchesCalleeName(current.callee, ONYX_ID_NORMALIZER_NAMES)) { + current = current.arguments.at(0); + } + + while (current && TYPE_ONLY_EXPRESSIONS.has(current.type)) { + current = current.expression; + } + + if (current?.type !== 'Identifier') { + return false; + } + + const variable = getVariableByName(scope, current.name); + + if (variable?.defs.length !== 1) { + return false; + } + + const definition = variable.defs.at(0); + + if (definition.type !== 'Variable' || definition.parent?.kind !== 'const') { + return false; + } + + const {id, init} = definition.node; + + if (id.type === 'Identifier') { + return isOnyxReadOf(init, CONCIERGE_REPORT_ID_KEY_PATH, scope); + } + + // const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID) + const firstElement = id.type === 'ArrayPattern' ? id.elements.at(0) : null; + return firstElement?.type === 'Identifier' && firstElement.name === current.name && isOnyxReadOf(init, CONCIERGE_REPORT_ID_KEY_PATH, scope); + } + + // `${ONYXKEYS.COLLECTION.REPORT}${conciergeReportID}`, directly or through a const + function isConciergeChatKey(node, scope, seen = new Set()) { + let current = node; + + while (current && TYPE_ONLY_EXPRESSIONS.has(current.type)) { + current = current.expression; + } + + if (current?.type === 'Identifier') { + if (seen.has(current)) { + return false; + } + + seen.add(current); + const initializer = getConstInitializer(current, scope); + return !!initializer && isConciergeChatKey(initializer, scope, 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 getOnyxKeyPath(current.expressions.at(0), scope) === CONCIERGE_CHAT_COLLECTION_KEY_PATH && isConciergeReportIDExpression(current.expressions.at(1), scope); + } + + return { + ...visitors, + CallExpression(node) { + const scope = sourceCode.getScope(node); + const readMethod = getCalledReadMethod(node.callee, scope); + + if (!readMethod || SNAPSHOT_AWARE_READ_FILES.has(getRepoRelativePath(filename))) { + return; + } + + const keyArgument = node.arguments.at(0); + + if (readMethod === SNAPSHOT_READ_METHOD) { + if (isConciergeChatKey(keyArgument, scope)) { + context.report({node, messageId: 'noConciergeChatInSnapshotReader'}); + return; + } + + const keyPath = getOnyxKeyPath(keyArgument, scope); + + if (!keyPath) { + context.report({node, messageId: 'noUnresolvableSnapshotReaderKey'}); + } else if (!RESTRICTED_KEY_PATHS.has(keyPath)) { + context.report({node, messageId: 'noNonSnapshotKeyInSnapshotReader', data: {keyPath: `${ONYXKEYS_ROOT}.${keyPath}`}}); + } + return; + } + + if (readMethod === READ_METHOD && isConciergeChatKey(keyArgument, scope)) { + return; + } + + for (const finding of findRestrictedKeys(readMethod, node, scope)) { + context.report( + finding.keyPath + ? {node: finding.node, messageId: 'noRestrictedOnyxKey', data: {keyPath: `${ONYXKEYS_ROOT}.${finding.keyPath}`}} + : {node: finding.node, messageId: 'noUnresolvableOnyxKey'}, + ); + } + }, + }; +} + +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..7065a3da5c69 --- /dev/null +++ b/eslint-plugin-local-rules/no-unsafe-onyx-read.js @@ -0,0 +1,477 @@ +// Node ESM needs the extension to load a rule's helper module +// eslint-disable-next-line import/extensions +import {createOnyxReadTracker, getCalleeName, getRepoRelativePath, getStaticName, matchesCalleeName} from './utils/onyxReadUtils.js'; + +const name = 'no-unsafe-onyx-read'; + +const READ_ALLOWED_DIRECTORIES = ['src/components/', 'src/pages/', 'src/hooks/', 'tests/']; + +const READ_ALLOWED_FILES = new Set([]); + +const EFFECT_HOOK_NAMES = new Set(['useEffect', 'useLayoutEffect', 'useInsertionEffect', 'useFocusEffect']); + +const CALLBACK_HOOK_NAMES = new Set(['useCallback']); + +// Hooks whose dependency list may name the useSnapshotOnyxGet() reader without calling it +const DEPENDENCY_LIST_HOOK_NAMES = new Set([...EFFECT_HOOK_NAMES, ...CALLBACK_HOOK_NAMES, 'useMemo', 'useImperativeHandle']); + +const EFFECT_CONTINUATION_NAMES = new Set(['then', 'catch', 'finally', 'setTimeout', 'requestAnimationFrame', 'queueMicrotask', 'runAfterInteractions', 'runAfterTransitions']); + +const SYNCHRONOUS_CALLBACK_METHODS = new Set(['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 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 unsafe Onyx reads: Onyx.get or Onyx.multiGet outside components, pages, hooks and tests, during render, inside effects or at module scope, and the useSnapshotOnyxGet() reader after it leaves the component. no-onyx-get-snapshot-key checks which keys are read.', + recommended: 'error', + }, + schema: [], + messages: { + noOnyxGetInRender: + 'Do not read Onyx during render. Onyx.get() and Onyx.multiGet() are one-shot reads that never subscribes, so a value they return while rendering does not re-render the component when that key changes and the UI can show stale data indefinitely. A component cannot await it either, so reaching it from render means use() or .then(), both of which read without subscribing.\n\n' + + 'Use useOnyx() for anything the component renders. Reserve Onyx.get() and Onyx.multiGet() for code that runs on an event: event handlers and useCallback bodies.', + noOnyxReadAtModuleScope: + 'Do not read Onyx at module scope. A module body runs at import time and cannot await, so the value can only be parked in a module variable through .then(), where it is a one-shot snapshot that never updates when the key changes.\n\n' + + 'Move the read inside the function that needs it, so it runs at event time and reads the current value. If the module genuinely needs to track a key, subscribe with Onyx.connectWithoutView() instead of caching one read.', + noEscapingSnapshotReader: + 'Call the useSnapshotOnyxGet() reader only from this component, inside its event handlers. Do not pass it to another component or function, return it, or store it anywhere but a local variable: this rule cannot follow it there, so it cannot stop a call during render or in an effect.\n\n' + + 'Call useSnapshotOnyxGet() in the component or hook that handles the event, or pass down a handler that calls the reader.', + noOnyxReadOutsideAllowedPath: + 'Onyx.get() and Onyx.multiGet() are only allowed in src/components, src/pages, src/hooks and tests.\n\n' + + 'Elsewhere, take the value as a parameter or keep the Onyx.connectWithoutView() subscription. A file joins READ_ALLOWED_FILES only in a PR that removes an Onyx.connectWithoutView() from it.', + noOnyxReadInEffect: + 'Do not read Onyx inside an effect, or in a function an effect calls. When the value was in the effect dependency array, the useOnyx subscription is what re-runs the effect, and a one-shot read stops that.\n\n' + + 'Keep the useOnyx subscription. Reads inside effects stay banned until a check can confirm the value was not in the dependency array.', + }, +}; + +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 getFunctionName(functionNode, parent) { + if (functionNode.id?.type === 'Identifier') { + return functionNode.id.name; + } + + if (parent?.type === 'VariableDeclarator' && parent.id.type === 'Identifier') { + return parent.id.name; + } + + if (parent?.type === 'Property' && !parent.computed && parent.key?.type === 'Identifier') { + return parent.key.name; + } + + return null; +} + +function returnsJSX(functionNode) { + const body = functionNode.body; + + if (!body) { + return false; + } + + if (body.type === 'JSXElement' || body.type === 'JSXFragment') { + return true; + } + + if (body.type !== 'BlockStatement') { + return false; + } + + return body.body.some((statement) => statement.type === 'ReturnStatement' && (statement.argument?.type === 'JSXElement' || statement.argument?.type === 'JSXFragment')); +} + +function getRenderTimeArgumentIndices(callee) { + const calleeName = getCalleeName(callee); + + return calleeName ? (RENDER_TIME_HOOK_ARGUMENTS.get(calleeName) ?? null) : null; +} + +function isHookOption(property) { + const call = property.parent?.parent; + + if (call?.type !== 'CallExpression' || !call.arguments.includes(property.parent)) { + return false; + } + + const calleeName = getCalleeName(call.callee); + + return !!calleeName && isHookName(calleeName); +} + +function isRenderTimeUsage(identifier) { + const parent = identifier.parent; + + if (parent?.type === 'Property' && parent.value === identifier && RENDER_TIME_OPTION_NAMES.has(getStaticName(parent.key, parent.computed)) && isHookOption(parent)) { + 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)); +} + +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}; + } + + return null; +} + +function isReferencedAtRenderTime(declaration, boundName, sourceCode, seen = new Set()) { + const variable = sourceCode.getDeclaredVariables(declaration).find((declaredVariable) => declaredVariable.name === boundName); + + if (!variable || seen.has(variable)) { + return false; + } + + seen.add(variable); + + return variable.references.some((reference) => { + const identifier = reference.identifier; + + if (isRenderTimeUsage(identifier)) { + return true; + } + + const parent = identifier.parent; + + if (parent?.type !== 'VariableDeclarator' || parent.init !== identifier || parent.id.type !== 'Identifier') { + return false; + } + + return isReferencedAtRenderTime(parent, parent.id.name, sourceCode, seen); + }); +} + +function isEffectCallback(functionNode, parent, grandparent) { + if (parent?.type !== 'CallExpression' || parent.arguments.at(0) !== functionNode) { + return false; + } + + if (matchesCalleeName(parent.callee, EFFECT_HOOK_NAMES)) { + return true; + } + + return ( + matchesCalleeName(parent.callee, CALLBACK_HOOK_NAMES) && + grandparent?.type === 'CallExpression' && + grandparent.arguments.at(0) === parent && + matchesCalleeName(grandparent.callee, EFFECT_HOOK_NAMES) + ); +} + +function getHandlerBinding(functionNode, parent, grandparent) { + const binding = getFunctionBinding(functionNode, parent); + + if (binding) { + return binding; + } + + if ( + parent?.type === 'CallExpression' && + parent.arguments.at(0) === functionNode && + matchesCalleeName(parent.callee, CALLBACK_HOOK_NAMES) && + grandparent?.type === 'VariableDeclarator' && + grandparent.init === parent && + grandparent.id.type === 'Identifier' + ) { + return {declaration: grandparent, name: grandparent.id.name}; + } + + return null; +} + +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))) + ); +} + +function isReachedFromEffect(ancestors, fromIndex, sourceCode, seen) { + function isHandlerInvokedFromEffect(binding) { + const variable = sourceCode.getDeclaredVariables(binding.declaration).find((declaredVariable) => declaredVariable.name === binding.name); + + if (!variable || seen.has(variable)) { + return false; + } + + seen.add(variable); + + return variable.references.some((reference) => { + if (!reference.isRead()) { + return false; + } + + const identifier = reference.identifier; + const parent = identifier.parent; + + if (parent?.type === 'CallExpression' && parent.arguments.at(0) === identifier && isEffectCallback(identifier, parent, parent.parent)) { + return true; + } + + const isCalled = parent?.type === 'CallExpression' && parent.callee === identifier; + + if (!isCalled && !runsWithEnclosingCode(identifier, parent)) { + return false; + } + + const referenceAncestors = sourceCode.getAncestors(identifier); + + 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, grandparent); + + if (binding) { + return isHandlerInvokedFromEffect(binding); + } + + if (!runsWithEnclosingCode(ancestor, parent)) { + return false; + } + } + + return false; +} + +function isReadAllowedInFile(filename) { + const relativePath = getRepoRelativePath(filename); + + if (relativePath === null) { + return true; + } + + return READ_ALLOWED_DIRECTORIES.some((directory) => relativePath.startsWith(directory)) || READ_ALLOWED_FILES.has(relativePath); +} + +function classifyFunctionBoundary(functionNode, parent, sourceCode) { + if (parent?.type === 'Property' && parent.value === functionNode && RENDER_TIME_OPTION_NAMES.has(getStaticName(parent.key, parent.computed)) && isHookOption(parent)) { + 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; + } + + return DEFERRED; + } + } + + const binding = getFunctionBinding(functionNode, parent); + + if (binding && isReferencedAtRenderTime(binding.declaration, binding.name, sourceCode)) { + return RENDER; + } + + const functionName = getFunctionName(functionNode, parent); + + if (functionName && (isHookName(functionName) || isComponentName(functionName))) { + return RENDER; + } + + return returnsJSX(functionNode) ? RENDER : DEFERRED; +} + +function classifyPosition(ancestors, sourceCode) { + 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); + + if (disposition === DEFERRED) { + return isReachedFromEffect(ancestors, index, sourceCode, new Set()) ? EFFECT : EVENT; + } + + if (disposition === RENDER) { + return RENDER; + } + } + + return MODULE_SCOPE; +} + +function create(context) { + const sourceCode = context.sourceCode ?? context.getSourceCode(); + const filename = context.filename ?? context.getFilename(); + const {visitors, getCalledReadMethod, isSnapshotReaderHookCall, snapshotReaderVariables} = createOnyxReadTracker(sourceCode); + + // The reader may only be called, copied to another local variable, or listed as a hook dependency + function isAllowedSnapshotReaderReference(identifier) { + const parent = identifier.parent; + + if (parent.type === 'CallExpression') { + return parent.callee === identifier; + } + + if (parent.type === 'VariableDeclarator') { + return parent.init === identifier && parent.id.type === 'Identifier'; + } + + if (parent.type === 'ArrayExpression') { + const hookCall = parent.parent; + return hookCall.type === 'CallExpression' && hookCall.arguments.includes(parent) && matchesCalleeName(hookCall.callee, DEPENDENCY_LIST_HOOK_NAMES); + } + + return false; + } + + return { + ...visitors, + CallExpression(node) { + const scope = sourceCode.getScope(node); + + // useSnapshotOnyxGet() itself: its result must be bound to a variable or called on the spot + if (isSnapshotReaderHookCall(node, scope)) { + const parent = node.parent; + const isBound = parent.type === 'VariableDeclarator' && parent.init === node && parent.id.type === 'Identifier'; + const isCalled = parent.type === 'CallExpression' && parent.callee === node; + + if (!isBound && !isCalled) { + context.report({node, messageId: 'noEscapingSnapshotReader'}); + } + return; + } + + if (!getCalledReadMethod(node.callee, scope)) { + return; + } + + if (!isReadAllowedInFile(filename)) { + context.report({node, messageId: 'noOnyxReadOutsideAllowedPath'}); + return; + } + + const position = classifyPosition(sourceCode.getAncestors(node), sourceCode); + + if (position === MODULE_SCOPE) { + context.report({node, messageId: 'noOnyxReadAtModuleScope'}); + return; + } + + if (position === RENDER) { + context.report({node, messageId: 'noOnyxGetInRender'}); + return; + } + + if (position === EFFECT) { + context.report({node, messageId: 'noOnyxReadInEffect'}); + } + }, + 'Program:exit': function () { + for (const variable of snapshotReaderVariables) { + for (const reference of variable.references) { + if (!reference.init && !isAllowedSnapshotReaderReference(reference.identifier)) { + context.report({node: reference.identifier, messageId: 'noEscapingSnapshotReader'}); + } + } + } + }, + }; +} + +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..f8ed6860444b --- /dev/null +++ b/eslint-plugin-local-rules/utils/onyxReadUtils.js @@ -0,0 +1,398 @@ +import fs from 'fs'; +import path from 'path'; + +const ONYX_MODULE = 'react-native-onyx'; + +const SNAPSHOT_KEYS_SOURCE = 'src/CONST/runtimeConfigured.ts'; + +const SNAPSHOT_KEYS_DECLARATION = /SEARCH_SNAPSHOT_ONYX_KEYS:\s*\[([^\]]*)\]/; + +const ONYXKEYS_ROOT = 'ONYXKEYS'; + +function findRepoRoot() { + let current = path.resolve(process.cwd()); + + while (true) { + if (fs.existsSync(path.join(current, SNAPSHOT_KEYS_SOURCE))) { + return current; + } + + const parent = path.dirname(current); + + if (parent === current) { + return null; + } + + current = parent; + } +} + +const REPO_ROOT = findRepoRoot(); + +function resolveRestrictedKeyPaths() { + const repoRoot = REPO_ROOT; + + if (!repoRoot) { + throw new Error(`no-unsafe-onyx-read and no-onyx-get-snapshot-key could not locate ${SNAPSHOT_KEYS_SOURCE}. Without it the rule would silently stop refusing Search snapshot keys.`); + } + + const source = fs.readFileSync(path.join(repoRoot, SNAPSHOT_KEYS_SOURCE), 'utf8'); + const declaration = SNAPSHOT_KEYS_DECLARATION.exec(source); + + if (!declaration) { + throw new Error( + `no-unsafe-onyx-read and no-onyx-get-snapshot-key could not read SEARCH_SNAPSHOT_ONYX_KEYS from ${SNAPSHOT_KEYS_SOURCE}. Without it the rule would silently stop refusing Search snapshot keys.`, + ); + } + + return new Set([...declaration[1].matchAll(/ONYXKEYS\.([A-Z0-9_.]+)/g)].map((match) => match[1])); +} + +const RESTRICTED_KEY_PATHS = resolveRestrictedKeyPaths(); + +const READ_METHOD = 'get'; + +const MULTI_READ_METHOD = 'multiGet'; + +const READ_METHODS = new Set([READ_METHOD, MULTI_READ_METHOD]); + +// Marks variables that hold the reader returned by useSnapshotOnyxGet() +const SNAPSHOT_READ_METHOD = 'snapshotGet'; + +const SNAPSHOT_READER_HOOK_SOURCE = /(^|\/)useSnapshotOnyxGet$/; + +const TYPE_ONLY_EXPRESSIONS = new Set(['TSAsExpression', 'TSSatisfiesExpression', 'TSNonNullExpression', 'TSInstantiationExpression', 'TSTypeAssertion']); + +function getStaticName(keyNode, computed) { + if (!computed && keyNode.type === 'Identifier') { + return keyNode.name; + } + + if (keyNode.type === 'Literal' && typeof keyNode.value === 'string') { + return keyNode.value; + } + + return null; +} + +function getStaticPropertyName(memberExpression) { + return getStaticName(memberExpression.property, memberExpression.computed); +} + +function unwrapKeyExpression(node) { + if (node && TYPE_ONLY_EXPRESSIONS.has(node.type)) { + return unwrapKeyExpression(node.expression); + } + + if (node?.type !== 'TemplateLiteral') { + return node; + } + + const leadingExpression = node.expressions.at(0); + + // cspell:disable-next-line -- quasis is the ESTree name for the static chunks of a template literal + if (!leadingExpression || node.quasis.at(0)?.value.cooked !== '') { + return node; + } + + return unwrapKeyExpression(leadingExpression); +} + +function getVariableByName(scope, variableName) { + let currentScope = scope; + + while (currentScope) { + const variable = currentScope.variables.find((scopeVariable) => scopeVariable.name === variableName); + + if (variable) { + return variable; + } + + currentScope = currentScope.upper; + } + + return null; +} + +function getConstInitializer(node, scope) { + const variable = getVariableByName(scope, node.name); + + 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; +} + +function getOnyxKeyPath(node, scope, 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, scope); + + return initializer ? getOnyxKeyPath(initializer, scope, 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) { + 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 isOnyxModuleSource(sourceValue) { + return sourceValue === ONYX_MODULE; +} + +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; +} + +function getKeyListElements(node, scope, seen = new Set()) { + let current = node; + + while (current && TYPE_ONLY_EXPRESSIONS.has(current.type)) { + current = current.expression; + } + + if (current?.type === 'Identifier') { + if (seen.has(current)) { + return null; + } + + seen.add(current); + const initializer = getConstInitializer(current, scope); + + return initializer ? getKeyListElements(initializer, scope, seen) : null; + } + + return current?.type === 'ArrayExpression' ? current.elements : null; +} + +/** + * Tracks the Onyx default import, Onyx.get / Onyx.multiGet aliases and the reader returned by useSnapshotOnyxGet() in one + * file, so every rule that inspects Onyx reads recognizes the same calls. + */ +function createOnyxReadTracker(sourceCode) { + const onyxImportBindings = new WeakSet(); + const snapshotReaderHookBindings = new WeakSet(); + const readAliases = new WeakMap(); + const snapshotReaderVariables = []; + + function getDeclaredVariable(node, bindingName) { + return sourceCode.getDeclaredVariables(node).find((declaredVariable) => declaredVariable.name === bindingName); + } + + function trackImportBinding(node, bindingName) { + const variable = getDeclaredVariable(node, bindingName); + + if (variable) { + onyxImportBindings.add(variable); + } + } + + function trackReadAlias(node, bindingName, readMethod) { + const variable = getDeclaredVariable(node, bindingName); + + if (variable) { + readAliases.set(variable, readMethod); + } + } + + function trackSnapshotReader(node, bindingName) { + const variable = getDeclaredVariable(node, bindingName); + + if (variable) { + readAliases.set(variable, SNAPSHOT_READ_METHOD); + snapshotReaderVariables.push(variable); + } + } + + function isSnapshotReaderHookCall(node, scope) { + if (node?.type !== 'CallExpression' || node.callee.type !== 'Identifier') { + return false; + } + + const hookVariable = getVariableByName(scope, node.callee.name); + return !!hookVariable && snapshotReaderHookBindings.has(hookVariable); + } + + function getOnyxReadMethod(node, scope) { + if (node?.type !== 'MemberExpression' || node.object.type !== 'Identifier') { + return null; + } + + const propertyName = getStaticPropertyName(node); + + if (!READ_METHODS.has(propertyName)) { + return null; + } + + const objectVariable = getVariableByName(scope, node.object.name); + return !!objectVariable && onyxImportBindings.has(objectVariable) ? propertyName : null; + } + + function getCalledReadMethod(callee, scope) { + if (isSnapshotReaderHookCall(callee, scope)) { + return SNAPSHOT_READ_METHOD; + } + + const readMethod = getOnyxReadMethod(callee, scope); + + if (readMethod) { + return readMethod; + } + + const calleeVariable = callee.type === 'Identifier' ? getVariableByName(scope, callee.name) : null; + + return calleeVariable ? (readAliases.get(calleeVariable) ?? null) : null; + } + + const visitors = { + ImportDeclaration(node) { + if (typeof node.source.value === 'string' && SNAPSHOT_READER_HOOK_SOURCE.test(node.source.value)) { + for (const specifier of node.specifiers) { + if (specifier.type !== 'ImportDefaultSpecifier') { + continue; + } + + const variable = getDeclaredVariable(node, specifier.local.name); + + if (variable) { + snapshotReaderHookBindings.add(variable); + } + } + return; + } + + if (!isOnyxModuleSource(node.source.value)) { + return; + } + + for (const specifier of node.specifiers) { + if (specifier.type === 'ImportDefaultSpecifier' || specifier.type === 'ImportNamespaceSpecifier') { + trackImportBinding(node, specifier.local.name); + } + } + }, + VariableDeclarator(node) { + const scope = sourceCode.getScope(node); + + if (node.id.type === 'ObjectPattern' && node.init?.type === 'Identifier') { + const initVariable = getVariableByName(scope, node.init.name); + + if (!initVariable || !onyxImportBindings.has(initVariable)) { + return; + } + + for (const property of node.id.properties) { + if (property.type !== 'Property' || property.value.type !== 'Identifier') { + continue; + } + + const keyName = getStaticName(property.key, property.computed); + + if (READ_METHODS.has(keyName)) { + trackReadAlias(node, property.value.name, keyName); + } + } + return; + } + + if (node.id.type !== 'Identifier') { + return; + } + + if (isSnapshotReaderHookCall(node.init, scope)) { + trackSnapshotReader(node, node.id.name); + return; + } + + if (node.init?.type === 'Identifier') { + const aliasedVariable = getVariableByName(scope, node.init.name); + + if (aliasedVariable && onyxImportBindings.has(aliasedVariable)) { + trackImportBinding(node, node.id.name); + } + + if (aliasedVariable && readAliases.get(aliasedVariable) === SNAPSHOT_READ_METHOD) { + trackSnapshotReader(node, node.id.name); + return; + } + } + + const readMethod = getOnyxReadMethod(node.init, scope); + + if (readMethod) { + trackReadAlias(node, node.id.name, readMethod); + } + }, + }; + + return {visitors, getCalledReadMethod, isSnapshotReaderHookCall, snapshotReaderVariables}; +} + +export { + ONYXKEYS_ROOT, + REPO_ROOT, + RESTRICTED_KEY_PATHS, + READ_METHOD, + MULTI_READ_METHOD, + READ_METHODS, + SNAPSHOT_READ_METHOD, + TYPE_ONLY_EXPRESSIONS, + getStaticName, + getStaticPropertyName, + unwrapKeyExpression, + getVariableByName, + getConstInitializer, + getOnyxKeyPath, + getCalleeName, + matchesCalleeName, + getRepoRelativePath, + getKeyListElements, + createOnyxReadTracker, +}; diff --git a/package-lock.json b/package-lock.json index 3eb37d7e7423..ae4564f9f25c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -127,7 +127,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.116", + "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", @@ -37805,9 +37805,9 @@ } }, "node_modules/react-native-onyx": { - "version": "3.0.116", - "resolved": "https://registry.npmjs.org/react-native-onyx/-/react-native-onyx-3.0.116.tgz", - "integrity": "sha512-390oUHqYrPxEjdBtqYoay0FfIrT9zUqq2YsyqwNOC1xIXn6/+aIGycTJ0kIJEpkbIN7MN8IdMs3smwDMfiSvGg==", + "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", @@ -37816,7 +37816,8 @@ "lodash.clone": "^4.5.0", "lodash.pick": "^4.4.0", "lodash.transform": "^4.6.0", - "underscore": "^1.13.6" + "underscore": "^1.13.6", + "use-sync-external-store": "^1.6.0" }, "engines": { "node": ">=20.19.5", @@ -42620,7 +42621,9 @@ } }, "node_modules/use-sync-external-store": { - "version": "1.5.0", + "version": "1.7.0", + "resolved": "https://registry.npmjs.org/use-sync-external-store/-/use-sync-external-store-1.7.0.tgz", + "integrity": "sha512-6L+EeigHMQhdaIPNIFUKwfWJSwWFQ8gJbJ2DLOs5sDIegTwR9fRxvnM3uciHKjIZhFz+KAv2emhWMRvDmMcY8A==", "license": "MIT", "peerDependencies": { "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" diff --git a/package.json b/package.json index 6b353b310e9e..3b0a681039ff 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.116", + "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 index 7134732ade19..4aa50f348bc8 100644 --- a/scripts/checkOnyxConnectBypass.ts +++ b/scripts/checkOnyxConnectBypass.ts @@ -2,7 +2,8 @@ import {file} from 'bun'; /** - * Fails the lint run when a new inline `eslint-disable` bypasses the Onyx.connect() ban. + * Fails the lint run when a new inline `eslint-disable` silences one of the bans in `BANNED_RULES` + * (see `onyxConnectBypass.ts`). * * 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 @@ -19,22 +20,20 @@ import {file} from 'bun'; import {execFileSync} from 'node:child_process'; import path from 'node:path'; -import {collectDisableDirectivesFromSource, findNewBypasses} from './onyxConnectBypass'; +import type {BannedRule} from './onyxConnectBypass'; + +import {BANNED_RULES, 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[] { +/** Files among the lint targets that mention every one of `searchTerms`. */ +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', '-e', 'Onyx', '-e', 'connect', '-e', 'eslint-disable', '--', ...pathSpecs], - { - cwd: projectRoot, - encoding: 'utf8', - }, - ); + 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) { if (typeof error === 'object' && error !== null && 'status' in error && error.status === 1) { @@ -45,11 +44,13 @@ function findCandidateFiles(targets: string[]): string[] { } /** - * 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). + * Checks `targets` for new bypasses of `ban`, reporting any to stderr. + * Returns `true` if a new bypass was found. */ -async function checkOnyxConnectBypass(targets: string[]): Promise { - const candidates = findCandidateFiles(targets); +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; } @@ -58,17 +59,17 @@ async function checkOnyxConnectBypass(targets: string[]): Promise { await Promise.all( candidates.map(async (relativePath) => { const source = await file(path.join(projectRoot, relativePath)).text(); - return collectDisableDirectivesFromSource(source, relativePath.split(path.sep).join('/')); + return collectDisableDirectivesFromSource(source, relativePath, ban); }), ) ).flat(); - const newBypasses = findNewBypasses(suppressed); + const newBypasses = findNewBypasses(suppressed, ban); 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(ban.message); console.error('New bypasses found:'); for (const bypass of newBypasses) { console.error(` ${bypass.file}:${bypass.line}`); @@ -76,6 +77,11 @@ async function checkOnyxConnectBypass(targets: string[]): Promise { return true; } +async function checkOnyxConnectBypass(targets: string[]): Promise { + const results = await Promise.all(BANNED_RULES.map((ban) => checkBan(targets, ban))); + return results.some(Boolean); +} + if (import.meta.main) { checkOnyxConnectBypass(process.argv.slice(2)) .then((failed) => { diff --git a/scripts/onyxConnectBypass.ts b/scripts/onyxConnectBypass.ts index 993200680ad7..db293d99845b 100644 --- a/scripts/onyxConnectBypass.ts +++ b/scripts/onyxConnectBypass.ts @@ -1,15 +1,16 @@ /** - * Detection logic for new `eslint-disable` bypasses of the Onyx.connect() ban. + * Finds `eslint-disable` directives that silence one of the 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`. + * `rulesdir/no-onyx-get-snapshot-key` may be disabled, but only with a reason after `--`. * - * `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. + * A directive counts when it names the ban's rule, or when it is a blanket disable over a banned + * call: `Onyx.connect()` for the connect ban, `Onyx.get()` or `Onyx.multiGet()` for the read ban. + * For the OnyxUtils ban it counts only when it covers a runtime import of the module. Blanket + * disables over unrelated code, like the ones around ReportUtils, are ignored. * - * 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. + * Calls and imports are located in the Babel AST, so spacing, comments and parentheses cannot hide + * them the way they could from a text scan. */ import {parse} from '@babel/parser'; @@ -33,7 +34,76 @@ const GRANDFATHERED_BYPASSES = new Map([ ['src/libs/ReportNameUtils.ts', 2], ]); -/** A `no-onyx-connect` violation that an inline disable directive silenced. */ +type BannedRule = { + id: string; + name: string; + objects: Set; + methods: Set; + /** Bans runtime imports of these modules instead of `objects`/`methods` calls. */ + importSources?: Set; + grandfathered: Map; + appliesTo: (file: string) => boolean; + searchTerms: string[]; + message: string; + /** Allows a directive that names the rule when it explains itself after `--`, instead of banning every disable. */ + requiresReason?: boolean; +}; + +const ONYX_CONNECT_BAN: BannedRule = { + id: BANNED_RULE_ID, + name: BANNED_RULE_NAME, + objects: new Set(['Onyx']), + methods: new Set(['connect']), + grandfathered: GRANDFATHERED_BYPASSES, + appliesTo: () => true, + searchTerms: ['Onyx', 'connect', 'eslint-disable'], + message: 'Onyx.connect() is banned and the ban cannot be bypassed with eslint-disable. Use the useOnyx() hook to read Onyx data instead.', +}; + +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']), + grandfathered: new Map([['src/setup/addUtilsToWindow.ts', 1]]), + appliesTo: (file) => file.startsWith('src/'), + searchTerms: ['Onyx', 'eslint-disable'], + message: + 'Onyx reads checked by no-unsafe-onyx-read cannot be silenced with eslint-disable. Fix the read instead: use useOnyx() for data a component renders or reacts to, and call Onyx.get() or Onyx.multiGet() only from event handlers or useCallback bodies in components, pages and hooks.', +}; + +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: (file) => file.startsWith('src/'), + searchTerms: ['OnyxUtils', 'eslint-disable'], + message: + 'The react-native-onyx/dist/OnyxUtils import restriction cannot be silenced with eslint-disable. Read Onyx with useOnyx(), Onyx.get() or Onyx.multiGet() instead. Type-only imports of OnyxUtils are still allowed.', +}; + +const ONYX_SNAPSHOT_KEY_BAN: BannedRule = { + id: 'rulesdir/no-onyx-get-snapshot-key', + name: 'no-onyx-get-snapshot-key', + objects: new Set(), + methods: new Set(), + grandfathered: new Map(), + appliesTo: (file) => file.startsWith('src/'), + searchTerms: ['no-onyx-get-snapshot-key', 'eslint-disable'], + requiresReason: true, + message: + 'Disables of no-onyx-get-snapshot-key need a reason after `--` that says why live data is right for this Search snapshot key, for example: `// eslint-disable-next-line rulesdir/no-onyx-get-snapshot-key -- acts on the live report, not the snapshot row`.', +}; + +const BANNED_RULES: BannedRule[] = [ONYX_CONNECT_BAN, ONYX_READ_BAN, ONYX_UTILS_IMPORT_BAN, ONYX_SNAPSHOT_KEY_BAN]; + +/** Shortest text after `--` that counts as a reason, so a token like `-- ok` does not pass. */ +const MIN_REASON_LENGTH = 12; + +/** A banned-rule violation that an inline disable directive silenced. */ type SuppressedBan = { file: string; line: number; @@ -115,7 +185,7 @@ function unwrapExpression(node: ASTNode): ASTNode { return current; } -function isOnyxConnectCall(node: ASTNode): boolean { +function isBannedCall(node: ASTNode, ban: BannedRule): 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. @@ -126,20 +196,58 @@ function isOnyxConnectCall(node: ASTNode): boolean { 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') { + if (!BabelASTUtils.isASTNode(callee.property) || callee.property.type !== 'Identifier' || typeof callee.property.name !== 'string' || !ban.methods.has(callee.property.name)) { return false; } if (!BabelASTUtils.isASTNode(callee.object)) { return false; } const object = unwrapExpression(callee.object); - return object.type === 'Identifier' && object.name === 'Onyx'; + return object.type === 'Identifier' && typeof object.name === 'string' && ban.objects.has(object.name); } -function collectOnyxConnectCallOffsets(root: ASTNode): number[] { +function collectBannedCallOffsets(root: ASTNode, ban: BannedRule): number[] { const offsets: number[] = []; const visit = (node: ASTNode) => { - if (isOnyxConnectCall(node)) { + if (isBannedCall(node, ban)) { + offsets.push(node.start); + } + for (const child of BabelASTUtils.children(node, NON_CHILD_KEYS)) { + visit(child); + } + }; + visit(root); + return offsets; +} + +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 collectBannedImportOffsets(root: ASTNode, sources: Set): number[] { + const offsets: number[] = []; + const visit = (node: ASTNode) => { + const source = importedSource(node); + if (source !== null && sources.has(source) && !isTypeOnlyImport(node)) { offsets.push(node.start); } for (const child of BabelASTUtils.children(node, NON_CHILD_KEYS)) { @@ -157,17 +265,22 @@ function normalizedDirectiveArgs(args: string): string { .trim(); } -function directiveTargetsBan(args: string): boolean { +function directiveTargetsBan(args: string, ban: BannedRule): 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}`); + return rule === ban.id || rule === ban.name || rule.endsWith(`/${ban.name}`); }); } +function hasReason(args: string): boolean { + const reason = args.match(/--(?[\s\S]*)$/)?.groups?.reason ?? ''; + return reason.replaceAll(/[\s*]+/g, ' ').trim().length >= MIN_REASON_LENGTH; +} + function isBlanketDirective(args: string): boolean { return normalizedDirectiveArgs(args).length === 0; } @@ -191,7 +304,7 @@ function lineNumberAtOffset(source: string, offset: number): number { return line; } -function blanketDirectiveCoversCall(source: string, match: DirectiveMatch, callOffsets: number[], enableMatches: DirectiveMatch[]): boolean { +function blanketDirectiveCoversCall(source: string, match: DirectiveMatch, callOffsets: number[], enableMatches: DirectiveMatch[], ban: BannedRule): boolean { const directiveLine = lineNumberAtOffset(source, match.index); const kind = directiveKind(match); const directiveEnd = match.index + match.text.length; @@ -213,29 +326,39 @@ function blanketDirectiveCoversCall(source: string, match: DirectiveMatch, callO return false; } const enableArgs = directiveArgs(enableMatch); - return isBlanketDirective(enableArgs) || directiveTargetsBan(enableArgs); + return isBlanketDirective(enableArgs) || directiveTargetsBan(enableArgs, ban); }); return !reenabled; }); } /** - * Find disable directives in `source` that suppress `rulesdir/no-onyx-connect`. + * Find disable directives in `source` that suppress `ban`. * Line numbers are 1-based. Matches both full-line and trailing `eslint-disable-line`. */ -function collectDisableDirectivesFromSource(source: string, file: string): SuppressedBan[] { +function collectDisableDirectivesFromSource(source: string, file: string, ban: BannedRule = ONYX_CONNECT_BAN): SuppressedBan[] { const parsed = parseSource(source); if (!parsed) { return []; } const bans: SuppressedBan[] = []; - const callOffsets = collectOnyxConnectCallOffsets(parsed.root); + if (ban.requiresReason) { + for (const match of collectDirectiveMatches(parsed.comments, source, 'disable')) { + const args = directiveArgs(match); + if (directiveTargetsBan(args, ban) && !hasReason(args)) { + bans.push({file, line: source.slice(0, match.index).split('\n').length}); + } + } + return bans; + } + const callOffsets = ban.importSources ? collectBannedImportOffsets(parsed.root, ban.importSources) : collectBannedCallOffsets(parsed.root, ban); 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) { + const targetsBan = directiveTargetsBan(args, ban); + const isBlanket = isBlanketDirective(args); + const coversBan = (isBlanket || (!!ban.importSources && targetsBan)) && blanketDirectiveCoversCall(source, match, callOffsets, enableMatches, ban); + if ((ban.importSources || !targetsBan) && !coversBan) { continue; } const prefix = source.slice(0, match.index); @@ -246,7 +369,7 @@ function collectDisableDirectivesFromSource(source: string, file: string): Suppr } /** Return the suppressed bans that exceed the grandfathered allowance for their file. */ -function findNewBypasses(suppressedBans: readonly SuppressedBan[]): SuppressedBan[] { +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) ?? []; @@ -256,7 +379,7 @@ function findNewBypasses(suppressedBans: readonly SuppressedBan[]): SuppressedBa const newBypasses: SuppressedBan[] = []; for (const [file, bans] of byFile) { - const allowed = GRANDFATHERED_BYPASSES.get(file) ?? 0; + const allowed = rule.grandfathered.get(file) ?? 0; if (bans.length <= allowed) { continue; } @@ -266,5 +389,16 @@ function findNewBypasses(suppressedBans: readonly SuppressedBan[]): SuppressedBa return newBypasses; } -export {BANNED_RULE_ID, BANNED_RULE_NAME, GRANDFATHERED_BYPASSES, collectDisableDirectivesFromSource, findNewBypasses}; -export type {SuppressedBan}; +export { + BANNED_RULE_ID, + BANNED_RULE_NAME, + BANNED_RULES, + GRANDFATHERED_BYPASSES, + ONYX_CONNECT_BAN, + ONYX_READ_BAN, + ONYX_SNAPSHOT_KEY_BAN, + ONYX_UTILS_IMPORT_BAN, + collectDisableDirectivesFromSource, + findNewBypasses, +}; +export type {BannedRule, SuppressedBan}; diff --git a/src/components/Search/SearchContext.tsx b/src/components/Search/SearchContext.tsx index ed4612cacf61..4f13b31fc6a9 100644 --- a/src/components/Search/SearchContext.tsx +++ b/src/components/Search/SearchContext.tsx @@ -10,6 +10,7 @@ import { SearchSelectionClearGenerationContext, SearchSelectionContext, SearchShiftRangeGroupsContext, + SearchSnapshotHashContext, } from './SearchContextDefinitions'; // Lightweight public surface for search contexts. @@ -60,6 +61,7 @@ export { SearchResultsActionsContext, SearchSelectionContext, SearchSelectionActionsContext, + SearchSnapshotHashContext, useSearchQueryContext, useSearchQueryActions, useSearchResultsContext, diff --git a/src/components/Search/SearchContextDefinitions.ts b/src/components/Search/SearchContextDefinitions.ts index da0373d7a13c..5fb6aea9c5a7 100644 --- a/src/components/Search/SearchContextDefinitions.ts +++ b/src/components/Search/SearchContextDefinitions.ts @@ -100,6 +100,8 @@ const SearchRowSelectionActionsContext = React.createContext(defaultSearchShiftRangeGroupsActions); /** Incremented whenever a clear empties the Search selection, so the shift+click range session knows to reset */ const SearchSelectionClearGenerationContext = React.createContext(0); +/** Hash of the active snapshot, or undefined when snapshot keys are read live. A number, so consumers re-render only when the active snapshot changes */ +const SearchSnapshotHashContext = React.createContext(undefined); export { EMPTY_TRANSACTIONS_BY_REPORT_ID, @@ -112,4 +114,5 @@ export { SearchRowSelectionActionsContext, SearchShiftRangeGroupsContext, SearchSelectionClearGenerationContext, + SearchSnapshotHashContext, }; diff --git a/src/components/Search/SearchResultsProvider.tsx b/src/components/Search/SearchResultsProvider.tsx index 1b8d5620bba8..6d4c993db90c 100644 --- a/src/components/Search/SearchResultsProvider.tsx +++ b/src/components/Search/SearchResultsProvider.tsx @@ -17,7 +17,7 @@ import {useOnyx} from 'react-native-onyx'; import type {SearchResultsActionsValue, SearchResultsContextValue} from './types'; import {useSearchQueryContext} from './SearchContext'; -import {EMPTY_TRANSACTIONS_BY_REPORT_ID, SearchResultsActionsContext, SearchResultsContext} from './SearchContextDefinitions'; +import {EMPTY_TRANSACTIONS_BY_REPORT_ID, SearchResultsActionsContext, SearchResultsContext, SearchSnapshotHashContext} from './SearchContextDefinitions'; type SearchResultsProviderProps = { children: React.ReactNode; @@ -104,9 +104,14 @@ function SearchResultsProvider({children}: SearchResultsProviderProps) { setLastSearchType, }; + // The condition useOnyx uses to read a snapshot key from snapshot_ + const snapshotHash = !shouldUseLiveData && !!currentSearchHash ? currentSearchHash : undefined; + return ( - {children} + + {children} + ); } diff --git a/src/hooks/useOnyx.ts b/src/hooks/useOnyx.ts index 323584c1c069..d05deff7350e 100644 --- a/src/hooks/useOnyx.ts +++ b/src/hooks/useOnyx.ts @@ -1,4 +1,4 @@ -import {SearchQueryContext, SearchResultsContext} from '@components/Search/SearchContext'; +import {SearchSnapshotHashContext} from '@components/Search/SearchContext'; import {useIsOnSearch} from '@components/Search/SearchScopeProvider'; import CONST from '@src/CONST'; @@ -14,7 +14,7 @@ import {useOnyx as useOnyxWithoutSnapshots} from 'react-native-onyx'; type UseOnyxWithoutSnapshots = typeof useOnyxWithoutSnapshots; const COLLECTION_VALUES = Object.values(ONYXKEYS.COLLECTION); -const getDataByPath = (data: SearchResults['data'], path: string) => { +const getDataByPath = (data: SearchResults['data'] | undefined, path: string) => { // Handle prefixed collections for (const collection of COLLECTION_VALUES) { if (path.startsWith(collection)) { @@ -28,7 +28,7 @@ const getDataByPath = (data: SearchResults['data'], path: string) => { }; // Helper function to get key data from snapshot -const getKeyData = (snapshotData: SearchResults, key: TKey): TReturnValue => { +const getKeyData = (snapshotData: OnyxEntry, key: TKey): TReturnValue => { if (key.endsWith('_')) { // Create object to store matching entries const result: OnyxCollection = {}; @@ -46,6 +46,11 @@ const getKeyData = (snapshotData: SearchResu return getDataByPath(snapshotData?.data, key) as TReturnValue; }; +/** Whether useOnyx reads this key from the active snapshot inside a Search scope */ +function isSnapshotCompatibleKey(key: OnyxKey): boolean { + return !key.startsWith(ONYXKEYS.COLLECTION.SNAPSHOT) && CONST.SEARCH.SNAPSHOT_ONYX_KEYS.some((snapshotKey) => key.startsWith(snapshotKey)); +} + /** * Resolves the final `useOnyx` result, extracting the specific key's data out of the search snapshot * when applicable. @@ -73,29 +78,22 @@ 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 isSnapshotKey = isSnapshotCompatibleKey(key); const isOnSearch = useIsOnSearch(); - let currentSearchHash: number | undefined; - let shouldUseLiveData = false; - if (isOnSearch && isSnapshotCompatibleKey) { - const {currentSearchHash: searchContextCurrentSearchHash} = use(SearchQueryContext); - const {shouldUseLiveData: contextShouldUseLiveData} = use(SearchResultsContext); - currentSearchHash = searchContextCurrentSearchHash; - shouldUseLiveData = !!contextShouldUseLiveData; - } + // Only a snapshot key inside a Search scope reads the hash, so other calls don't re-render when the search changes + const snapshotHash = isOnSearch && isSnapshotKey ? use(SearchSnapshotHashContext) : undefined; const useOnyxOptions = options as UseOnyxOptions> | undefined; const {selector: selectorProp, ...optionsWithoutSelector} = useOnyxOptions ?? {}; - // Determine if we should use snapshot data based on search state and key - const shouldUseSnapshot = isOnSearch && !!currentSearchHash && isSnapshotCompatibleKey && !shouldUseLiveData; + const shouldUseSnapshot = !!snapshotHash; // Create selector function that handles both regular and snapshot data const selector = !selectorProp || !shouldUseSnapshot ? selectorProp : (data: OnyxValue | undefined) => selectorProp(getKeyData(data as SearchResults, key)); const onyxOptions: UseOnyxOptions> = {...optionsWithoutSelector, selector}; - const snapshotKey = shouldUseSnapshot ? (`${ONYXKEYS.COLLECTION.SNAPSHOT}${currentSearchHash}` as OnyxKey) : key; + const snapshotKey = shouldUseSnapshot ? (`${ONYXKEYS.COLLECTION.SNAPSHOT}${snapshotHash}` as OnyxKey) : key; const originalResult = useOnyxWithoutSnapshots(snapshotKey, onyxOptions); @@ -106,3 +104,4 @@ const useOnyx: UseOnyxWithoutSnapshots = (key: TKey): Promise> => { + if (!snapshotHash || !isSnapshotCompatibleKey(key)) { + return Onyx.get(key); + } + + const snapshot = await Onyx.get(`${ONYXKEYS.COLLECTION.SNAPSHOT}${snapshotHash}`); + return getKeyData>(snapshot, key); + }; +} + +export default useSnapshotOnyxGet; 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..51967585db54 --- /dev/null +++ b/tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts @@ -0,0 +1,304 @@ +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; + +import type {Rule} from 'eslint'; + +import {Linter, RuleTester} from 'eslint'; +import path from 'path'; +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; +} + +const ruleModule: unknown = require('../../eslint-plugin-local-rules/no-onyx-get-snapshot-key'); + +if (!isLocalRuleModule(ruleModule)) { + throw new TypeError('Expected no-onyx-get-snapshot-key to export an ESLint rule module.'); +} + +const localRule: LocalRuleModule = ruleModule; + +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 SNAPSHOT_READER_IMPORT = "import useSnapshotOnyxGet from '@hooks/useSnapshotOnyxGet';"; + +const REPO_ROOT = path.resolve(__dirname, '../..'); + +function inRepo(relativePath: string): string { + return path.join(REPO_ROOT, relativePath); +} + +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); }`}, + + {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(ONYXKEYS.SESSION); }`}, + ], + 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() { 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, + }, + ], + }); +}); + +describe('no-onyx-get-snapshot-key useSnapshotOnyxGet reader', () => { + ruleTester.run(ruleModule.name, ruleModule, { + valid: [ + // Snapshot keys read from handlers + { + code: `${SNAPSHOT_READER_IMPORT} function Row({reportID}) { const getOnyx = useSnapshotOnyxGet(); const onPress = async () => getOnyx(\`\${ONYXKEYS.COLLECTION.REPORT}\${reportID}\`); return