Catch unhandled rejection on fire-and-forget AsyncStorage.setItem#119
Merged
Merged
Conversation
## Vulnerability counts — both clean
| Tool | Before | After |
| ------------------- | ---------------------------- | ----- |
| `yarn audit` (root) | 19 unique (moderate + high) | 0 |
| `yarn audit` (example) | 155 (44 moderate / 111 high) | 0 |
## Approach
Followed the pattern from `@eppo/js-client-sdk-common@5.0.0`: bump direct
deps (taking major versions where reasonable) and drop unused deps that
were dragging in vulnerable transitives. Only two new `resolutions`
entries — both in `example/` for transitives buried under `expo` that
have no path-through fix at SDK 54.
## Production dep changes
- `@eppo/js-client-sdk-common` 4.15.1 → 5.0.0
- The 5.0.0 release of the common SDK was cut to clear vulnerabilities
— it drops `uuid` entirely (replaced with `crypto.randomUUID`), which
kills the only production vuln in this repo.
- The matching `engines.node` floor (>=18 → >=20) is mirrored here,
driving this SDK's major bump.
- `@react-native-async-storage/async-storage` ^1.18.0 → ^2.2.0 (the
surface we use — `getItem`/`setItem`/`removeItem` — is unchanged).
## Dev dep cleanup
Removed entirely (unused — these were the source of the bulk of the
audit findings):
- `express` + `@types/express` — never imported in src/, scripts/, or
test setup. Source of `path-to-regexp` and `qs` advisories.
- `del-cli`, `pod-install` — declared but not invoked from any script.
- `@types/react-native` — RN ships its own types since 0.71;
`@types/react-native` is officially a deprecated stub.
- `@react-native-community/eslint-config` — pulled in old
`@typescript-eslint` v5, `eslint-plugin-prettier@^4`, `babel/eslint-parser`,
etc., all of which carried vulnerable transitives. The only rule we
actually relied on was `prettier/prettier`. Replaced with a minimal
`eslint:recommended` + `@typescript-eslint/recommended` + `prettier`
setup with the same prettier options.
- `metro-react-native-babel-preset` (via babel.config.js) — replaced
with `@react-native/babel-preset` (the RN 0.73+ standard, ships with
the RN devDep).
Bumped:
- `react-native` 0.71.3 → 0.85.3 + `react` 18.2 → 19.2 + `@types/react`
17 → 19 (devDeps only — the published peer range stays at `*`).
- `@react-native/{babel-preset,jest-preset}` 0.85.3 added explicitly so
the jest preset reference resolves and we can drop the deprecated
built-in `react-native` preset.
- `jest` 29 → 30, `@types/jest` 28 → 30 — clears the `glob`/`picomatch`
chain.
- `@typescript-eslint/{eslint-plugin,parser}` added at 8.59.1 — clears
the rest of the `minimatch` chain.
- `eslint-config-prettier` 8 → 10, `eslint-plugin-prettier` 4 → 5,
`prettier` 2 → 3.
- `typescript` 4.9 → 5.9 — required for `verbatimModuleSyntax`
(replacement for the now-removed `importsNotUsedAsValues`).
- `react-native-builder-bob` 0.20 → 0.41 — config syntax is compatible;
it now reads babel via `@react-native/babel-preset`.
## Source/config touch-ups driven by the toolchain bumps
- `tsconfig.json`: `importsNotUsedAsValues: "error"` (removed in TS 5.5)
→ `verbatimModuleSyntax: true`.
- `src/index.tsx`, `src/__tests__/index.test.tsx`,
`src/cache/simple-assignment-cache.ts`: split mixed type/value imports
so type-only imports use `import type` (required by
`verbatimModuleSyntax`); same treatment for type-only re-exports in
`src/index.tsx`.
- `src/cache/async-storage-assignment-shim.ts`: dropped explicit
`IterableIterator<…>` return types on `entries`/`keys`/`values`/
`[Symbol.iterator]`. TS 5.x lib types declare `Map`'s versions as
`MapIterator<…>`; inferring the return type lets the override match
the base class without us hard-coding the new name.
- ESLint config: existing call sites in `check-types.js`,
`scripts/bootstrap.js`, `jestSetup.js`, `src/sdk-data.ts` use CJS
`require()` (legitimately — these run as Node scripts under
`metro-react-native-babel-preset` historically). Turned off
`@typescript-eslint/no-require-imports` and `no-var`, kept
`no-explicit-any` off (the codebase has a few intentional `any`s),
and ignored `example/` from root lint since Expo runs its own ESLint
config there.
## Example app
- Added two `resolutions`: `postcss@^8.5.10` and `uuid@^11.1.1`. Both
are transitives reachable only via the `expo` package (postcss via
`@expo/metro-config`, uuid via `@expo/config-plugins > xcode`). No
expo SDK 54.x patch ships either at a fixed version yet, and bumping
the entire Expo SDK family (54 → 56) for a demo app is more risk than
the targeted resolutions. These resolutions don't propagate to SDK
consumers since `example/` is a private, non-published workspace.
## Engines / CI / .nvmrc
- `engines.node` `>=18.0.0` → `>=20.0.0`.
- `.nvmrc` `16.20.2` → `20`.
- CI (`ci.yml`, `publish.yaml`) `node-version: '18'`/`'18.x'` → `'20'`/
`'20.x'`.
## Version bump
Treating this as **4.0.0** because of the `engines.node` floor change
(>=18 → >=20), matching the SemVer-major precedent from
`@eppo/js-client-sdk-common@5.0.0` and `@eppo/node-server-sdk@4.0.0`.
Public TS surface and runtime behavior are unchanged.
## Test plan
- [x] `yarn audit` clean in root (0 / 753 packages).
- [x] `yarn audit` clean in `example/` (0 / 1003 packages).
- [x] `yarn typecheck` passes.
- [x] `yarn test`: 38 / 38 pass across 2 suites.
- [x] `yarn lint`: 0 errors after auto-fix.
- [x] `yarn prepack` (bob build + check-types): commonjs, module, and
typescript targets all written.
- [x] `npx expo export` succeeds for `web`, `ios`, and `android`
platforms in `example/`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The reusable setup action in .github/actions/setup/action.yml had its own hard-coded `node-version: '18.x'`, which overrode the `'20'` set in the job-level setup-node step earlier in ci.yml. The first install failed on `engines.node: ">= 20.0.0"` because the composite reinstalled Node 18 right after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot suggested keeping `no-var` enabled to prevent stray `var` declarations from slipping into library code. Repo only had one `var` (in a `for...in` loop in `src/async-storage.ts`), so converted that to `const` and dropped the rule override. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ESUPPORT-730] The persistent-write side of `AsyncStorageStore.setEntries` is intentionally fire-and-forget — the in-memory cache is populated synchronously so assignments serve immediately, while persistence happens in the background. Without a `.catch()` handler, a persist failure surfaces as an unhandled promise rejection in the host app. Mirrors the existing `.catch(console.error)` pattern in `src/cache/async-storage-assignment-shim.ts` (lines 28, 33). No behavior change beyond removing the unhandled rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents unhandled promise rejections in host apps by explicitly handling failures from a fire-and-forget AsyncStorage.setItem call used to persist the in-memory cache.
Changes:
- Add a rejection handler (
.catch(console.error)) to theAsyncStorage.setItemcall inAsyncStorageStore.setEntriesto avoid unhandled promise rejections when persistence fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
greghuels
approved these changes
May 26, 2026
Base automatically changed from
aarsilv/ffesupport-728/address-vulnerabilities
to
main
May 26, 2026 11:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FFESUPPORT-730 · stacked on #118
Summary
Adds a
.catch(console.error)to the fire-and-forgetAsyncStorage.setItemcall inAsyncStorageStore.setEntries. Without it, a persist failure surfaces as an unhandled promise rejection in the host app.Mirrors the same pattern already used in
src/cache/async-storage-assignment-shim.ts:28,33. Pre-existing latent bug, not a regression from #118 — flagged by Copilot as a low-confidence observation during the #118 review.Test plan
yarn typecheck,yarn lint,yarn test(38/38) all pass.🤖 Generated with Claude Code