fix(analytics-react-native): lazy-load AsyncStorage so customers can opt out (SDKRN-8)#1772
Conversation
…opt out (SDKRN-8) Replace the static `import AsyncStorage from '@react-native-async-storage/async-storage'` at the top of `local-storage.ts` with a lazy `require` wrapped in try/catch. When the package isn't resolvable (e.g. the customer has supplied their own `storageProvider` and excluded AsyncStorage from native autolinking via `react-native.config.js`), `LocalStorage.isEnabled()` returns false and the existing fallback chain in `createFlexibleStorage` drops down to `MemoryStorage` instead of throwing at module-load. Document the opt-out recipe in the README. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
size-limit report 📦
|
…KRN-8) - Document why getAsyncStorage caches resolution permanently (no retry): require() is synchronous and RN's module registry is stable for the app lifetime, so retrying after failure would never produce a different result. - Strengthen the "no AsyncStorage installed" test: factory is now a jest.fn() that we assert is called exactly once, proving each storage method short-circuits on the cached undefined value instead of re-attempting require() or invoking methods on undefined. - Fix README: Metro does not tree-shake. Reword RN Web step to point at webpack aliasing instead. - Consolidate the four eslint-disable-next-line comments into a single block directive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the React Native SDK’s local storage implementation to avoid crashing at module-load time when @react-native-async-storage/async-storage is not available/linked, enabling customers to opt out of AsyncStorage when providing a custom storageProvider.
Changes:
- Replaced the static AsyncStorage import with a lazy
require()(cached) guarded by availability checks. - Added a Jest test to ensure
LocalStoragebecomes a no-op when AsyncStorage cannot be resolved. - Documented the recommended opt-out steps (custom
storageProvider+ native autolinking exclusion) in the RN package README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/analytics-react-native/src/storage/local-storage.ts | Lazy-resolve AsyncStorage and short-circuit storage operations when unavailable. |
| packages/analytics-react-native/test/storage/local-storage.test.ts | Adds coverage for the “module cannot be resolved” degradation path. |
| packages/analytics-react-native/README.md | Documents how to opt out of AsyncStorage in native builds while using a custom provider. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…RN-8)
- Add try/catch in LocalStorage.getRaw so direct callers like
parseOldCookies (cookie-migration/index.ts:10) don't propagate a native
bridge "AsyncStorage is null" error when the JS package is present but
the native module is opted out.
- Discriminate require() errors in getAsyncStorage: only MODULE_NOT_FOUND
is silently swallowed (the supported opt-out path). Any other error
(broken install, syntax error, permission issue) is surfaced via
console.warn so misconfigurations can be diagnosed instead of silently
degrading to in-memory storage.
- Test additions:
- Update the "module not resolvable" test to throw a real MODULE_NOT_FOUND
error and assert no warning is logged.
- New test: non-MODULE_NOT_FOUND errors produce a warning and still
degrade to no-op.
- New test: getRaw swallows native bridge errors when the JS package is
resolvable but methods reject.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ut recipe (SDKRN-8) Address Copilot review comments 1 and 2 from round 2: - Customers who only override `storageProvider` still hit AsyncStorage via the default cookieStorage fallback chain. Document both slots and the trade-off if only one is overridden (identity degrades to in-memory and resets per launch). - Soften the "never invoked" claim: AsyncStorage methods aren't invoked only when both storage slots are overridden. The JS package staying in node_modules is just about `require()` resolvability, not call patterns. Pushed back on comment 3 (silent MODULE_NOT_FOUND) in the PR thread — broken-install case is already covered via console.warn for non-MNF errors; silent fallback is intentional for the supported opt-out path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ckage only (SDKRN-8) Address Copilot review round 3: Node sets `code === 'MODULE_NOT_FOUND'` for *any* missing module in the require() chain — including transitive deps of @react-native-async-storage/ async-storage itself. The previous check would silently disable persistence on a broken transitive install, masking the bug. Tighten the silent-opt-out condition: the error message must specifically mention `@react-native-async-storage/async-storage`. Anything else (broken transitive, syntax error, etc.) hits the console.warn path so customers can diagnose it. Add a test for the transitive-MODULE_NOT_FOUND scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@bug Bot |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e187a6f. Configure here.
…stro (SDKRN-8)
Make the React Native example app a regression test for the SDKRN-8 opt-out
recipe. The app now:
- Excludes RNCAsyncStorage from native autolinking via
react-native.config.js (so the iOS / Android binary doesn't link it).
- Overrides both `storageProvider` and `cookieStorage` with an in-memory
Storage implementation defined inline in App.tsx, fully bypassing the
SDK's default LocalStorage chain.
- Calls init with the correct (apiKey, userId, options) signature — pass
`undefined` for userId so the overrides land in the options slot rather
than being silently bound to userId.
Extend .maestro/smoke.yaml to tap both Track Event and Track Identify
buttons and assert each one produces an "Amplitude Response" toast. If the
SDK regresses to a static `import AsyncStorage` or any code path that
touches the native module without graceful handling, the app crashes at
boot with `[@RNC/AsyncStorage]: NativeModule: AsyncStorage is null` and
none of the new assertions can pass.
Validated locally on iPhone 16 simulator (iOS 18.6) — Release build, all
Maestro steps pass, simctl log confirms no AsyncStorage errors. The
`.github/workflows/rn-smoke.yml` workflow runs the same flow on iPhone 15
(macos-14), so the regression guard ships with the PR.
Also fix the README opt-out snippet to use the correct three-arg init
signature (apiKey, userId, options) — the previous two-arg example was
misleading and would silently fall back to the default storage chain.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…js (SDKRN-8) The example app's eslint+prettier config strips spaces inside inline object literals. Match the rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n (SDKRN-8) The smoke.yaml comment claimed @react-native-async-storage/async-storage was removed from the example app's package.json. It isn't — we kept it in the example app's deps so the lockfile doesn't churn. The opt-out is delivered entirely by the autolinking exclusion + storage-slot overrides. Update the comment to reflect that, and add an explicit note that the JS package is still resolvable but the native module is null — which is the trigger condition for the regression we're guarding against. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Re: code-reviewer Should-fix on The reviewer flagged async getRaw(key: string): Promise<string | undefined> {
const value = await this.get(key);
return value ? JSON.stringify(value) : undefined;
}My The "double-encoded string" concern is also moot in practice for this example app: Addressed Nit 1 (toast lingering between Track Event and Track Identify) in the next commit by waiting for the first toast to dismiss before tapping. Locally re-verified — all Maestro steps pass. Keeping Nit 2 (README "Common gotchas" SSR note) as-is — RN Web in Next.js is a real customer setup and the gotcha is genuine for those users. Trimming the note would lose useful information. |
…ps (SDKRN-8) Address code-reviewer Nit: the second `extendedWaitUntil: Amplitude Response` could be satisfied by the lingering toast from the previous tap instead of waiting for a new toast from `Track Identify`. Add an explicit `extendedWaitUntil: notVisible` between the two button taps so the second assertion actually proves the second SDK call produced its own toast. react-native-toast-message auto-dismisses after ~4s, well within the 10s timeout. Pushback on the Should-fix (InMemoryStorage.getRaw) is in the PR thread — the pattern matches the SDK's own canonical `MemoryStorage<T>`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f42f4b4. Configure here.
Mercy811
requested review from
aliaksandr-kazarez,
crleona and
daniel-graham-amplitude
…Maestro flakiness (SDKRN-8) The previous App.tsx tapped track()/identify() and showed the toast only inside the .then of the returned promise. That promise resolves once the event flushes to api2.amplitude.com — fast on a dev mac (~1-3s) but slow or unreachable on macos-14 CI runners. The Maestro flow timed out waiting for the toast in the CI iOS smoke job: Tap on "Track Event"... COMPLETED Assert that "Amplitude Response" is visible... FAILED (10s timeout) The SDK itself is fine — sim logs show no AsyncStorage error and the title renders before the failed assertion. The flakiness is purely about network timing. Show the toast immediately on tap and route the SDK's eventual response (or rejection) to a follow-up toast. The regression we actually care about — that track()/identify() don't crash the app when AsyncStorage isn't linked — is what the immediate toast verifies. The follow-up toast still surfaces the real SDK response when network is healthy (local dev). Update the smoke.yaml comment to reflect that the assertion now proves the synchronous SDK call returned, not the full promise pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| if (resolved) { | ||
| return asyncStorage; | ||
| } | ||
| resolved = true; |
There was a problem hiding this comment.
Having a "resolved" variable is redundant. You can just say:
if (asyncStorage) {
return asyncStorage;
}
There was a problem hiding this comment.
I think I understand now, resolved could also mean it failed to load and we don't wish to try again.
There was a problem hiding this comment.
In a clean full-opt-out the scenario it is indeed dead code because getAsyncStorage() will never be called. But in the partial-opt-out case where, for example, it gets called from the cookieStorage fallback and async-storage is also missing, Metro doesn't cache failed resolutions, so without the flag we'd re-throw and re-log on every storage operation.
| resolved = true; | ||
| /* eslint-disable @typescript-eslint/no-var-requires, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access */ | ||
| try { | ||
| const mod = require('@react-native-async-storage/async-storage'); |
There was a problem hiding this comment.
Did you consider async ESM imports? (example: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import#import_a_module_for_its_side_effects_only)
There was a problem hiding this comment.
that's async, it's better to be sync to keep the existing storage methods clean.
| // | ||
| // The result is cached for the app lifetime: `require()` is synchronous in | ||
| // React Native and the module registry is stable, so retrying after a failed | ||
| // resolution would never produce a different result. |
There was a problem hiding this comment.
I think these comments are a tad verbose. A lot of this information could and should be gleaned from just looking at the source code.
There was a problem hiding this comment.
I'd still like to keep them just for readability if you don't have strong opinion on this. No harm of having comments IMHO
| // directly without a `default` wrapper. | ||
| asyncStorage = mod?.default ?? mod; | ||
| } catch (e) { | ||
| asyncStorage = undefined; |
There was a problem hiding this comment.
Optional: you could set this to null to indicate that it's not available at all, then you wouldn't need the resolved variable.
There was a problem hiding this comment.
I like this for simplicity! Updated at 79f9d9a
|
|
||
| AsyncStorage is no longer linked into your iOS or Android binaries. The JS package stays in `node_modules` so `require()` still resolves, but with both storage slots overridden, the SDK never invokes any AsyncStorage methods. | ||
|
|
||
| 3. For React Native Web, exclude AsyncStorage from your web bundle. |
There was a problem hiding this comment.
This feels like overkill to me. From looking at Bundlephobia it looks like async-storage has a min+GZ size of 0.25 kilobytes. Could we at least maybe move this to another .md that's something like an Advanced Usage markdown file?
There was a problem hiding this comment.
I will move them to dev doc center. It doesn't affect the mini file and the gzip size. It only affects the tarball size which adds download cost of npm install this package.
There was a problem hiding this comment.
|
@Mercy811 can I confirm that you were able to:
|
…e backend' Co-authored-by: Xinyi Ye <xinyi.ye@amplitude.com>
…a value Co-authored-by: Xinyi Ye <xinyi.ye@amplitude.com>
@daniel-graham-amplitude that's right, by the example app. And also added a maestro test to prevent regression too. Enriched PR description. |
…cs site (SDKRN-16) The opt-out recipe now lives in the React Native SDK developer docs: https://amplitude.com/docs/sdks/analytics/react-native/react-native-sdk#opt-out-of-asyncstorage Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rage (SDKRN-8) Address review feedback: collapse the `resolved` boolean and `asyncStorage` variable into a single tri-state variable using `null` to mark "tried and unavailable". Widen `getAsyncStorage`'s return type to `AsyncStorageLike | null | undefined` so callers (all in this file, all using `if (!storage)` truthy checks) don't need any changes — TypeScript narrows the union out on the falsy branch. Net change: one fewer module-level variable, no `?? undefined` adapter required at the return sites, identical behavior, identical test coverage (jest.fn factory still invoked exactly once on the failed-resolution path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ke test (SDKRN-8)
The previous smoke test relied on react-native.config.js to exclude
RNCAsyncStorage from autolinking. That config-driven path turns out not
to exercise the customer-reported failure mode — RN CLI / Metro stubs
the JS package when autolinking is excluded by config, so the package's
top-level NativeModule-null throw is never even in the bundle. The test
passed regardless of whether the SDK lazy-loaded the require.
Switch the example app to the actual customer-shaped recipe — also the
recipe we'll document on the dev docs site:
1. Don't depend on @react-native-async-storage/async-storage in your
own package.json. In a pnpm setup, this keeps the package out of
top-level node_modules, which keeps RN autolinking from finding
the native module, which keeps RNCAsyncStorage out of the iOS
binary (verifiable: `grep -c RNCAsyncStorage ios/Podfile.lock` = 0).
2. Pass your own storageProvider AND cookieStorage in init(). With
both slots overridden, the SDK never reaches the default
LocalStorage fallback chain, so the lazy require of async-storage
is never triggered.
Changes:
- Delete examples/react-native/app/react-native.config.js — no longer
needed (and was masking the actual failure mode).
- Drop @react-native-async-storage/async-storage from
examples/react-native/app/package.json.
- Regenerate pnpm-lock.yaml.
- Update .maestro/smoke.yaml header comments to describe the new recipe.
Validated locally on iPhone 16 simulator (iOS 18.6) — Release build,
Maestro flow runs to completion with all 9 steps passing, sim log shows
zero AsyncStorage / NativeModule errors. ios/Podfile.lock has 0
references to RNCAsyncStorage.
This is also a tighter regression guard: if the SDK regresses to its
pre-PR static \`import AsyncStorage from ...\`, the app will crash at
boot with \`[@RNC/AsyncStorage]: NativeModule: AsyncStorage is null\`,
which the Maestro flow will catch (the title assertion fails before any
tap step runs).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…L parse (SDKRN-8) Previous comment-block edit accidentally included a markdown blockquote character at the start of a comment line, which Maestro's YAML parser rejects: Parsing Failed at .../smoke.yaml:2:4 iOS Maestro CI failed for that reason on the last push. This is the one-character fix to unbreak the parse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…SDKRN-8)
Mirror the .env / VITE_AMPLITUDE_API_KEY pattern used by the Playwright
E2E tests (see .github/actions/e2e-test/action.yml). The Maestro smoke
test now sends events end-to-end to a real Amplitude project under our
test API key when running in CI.
Changes:
- examples/react-native/app/babel.config.js: enable
babel-plugin-transform-inline-environment-variables for the single
whitelisted var AMPLITUDE_API_KEY. The plugin inlines the value as a
string literal in the JS bundle at Metro bundle time.
- examples/react-native/app/App.tsx: read process.env.AMPLITUDE_API_KEY
with a 'YOUR_API_KEY' literal fallback so the example app is still
self-contained without any env set.
- examples/react-native/app/package.json + pnpm-lock.yaml: add
babel-plugin-transform-inline-environment-variables to devDependencies.
- .github/workflows/rn-smoke.yml: pass AMPLITUDE_API_KEY=${{
secrets.AMPLITUDE_API_KEY }} as env to the xcodebuild step. With no
secret, the fallback 'YOUR_API_KEY' is bundled and the crash-guard
Maestro assertions still pass; only the api2 round-trip is skipped.
Local dev:
export AMPLITUDE_API_KEY=… # then pnpm ios / xcodebuild
# (or use direnv for repo-scoped scoping)
Verified locally on iPhone 16 / iOS 18.6 Release build:
- With no env: bundle contains 'YOUR_API_KEY', no leaked real key.
- With env set: bundle contains the real key, 'YOUR_API_KEY' literal
is absent.
- Maestro flow runs end-to-end with all 9 steps passing in both cases.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…RN-8) Prettier wants the plugin tuple split across multiple lines. Auto-fix applied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Replace the static
import AsyncStorage from '@react-native-async-storage/async-storage'at the top ofpackages/analytics-react-native/src/storage/local-storage.tswith a lazyrequirewrapped in try/catch. When the package can't be resolved,LocalStorage.isEnabled()returnsfalseand the existing fallback chain increateFlexibleStoragedrops down toMemoryStorageinstead of throwing at module-load.Linear: SDKRN-8
Problem
@react-native-async-storage/async-storageis currently a regulardependencyand the SDK statically imports it. Customers who:config.storageProvider(e.g.react-native-mmkv), and…still hit:
Reported across #266, #540, #1283, #1595 and several customer support threads.
Reproduce steps
@react-native-async-storage/async-storageinpackage.jsoncd ios && bundle install && bundle exec pod installcd examples/react-native-app && pnpm startto start metropnpm iosWhat changed
packages/analytics-react-native/src/storage/local-storage.ts: lazy-resolve AsyncStorage inside agetAsyncStorage()helper. Each method checks for availability before calling. The require is wrapped in try/catch so a missing JS package no longer crashes module-load. The existing try/catch around individual storage calls still handles the "JS present but native module null" case.packages/analytics-react-native/README.md: document the opt-out recipe (customstorageProvider+react-native.config.jsautolinking exclusion).packages/analytics-react-native/test/storage/local-storage.test.ts: new test verifyingLocalStoragedegrades to a no-op when the package can't be resolved (jest.resetModules+jest.doMockwith a throwing factory).No
package.jsonchanges —@react-native-async-storage/async-storageremains a regulardependency, so the default install path is unchanged and existing customers keep their identity continuity.Customer opt-out recipe (now documented in README)
The JS package stays in
node_modules(~1 KB) but is never invoked; the iOS and Android binaries no longer include AsyncStorage.Out of scope
Test plan
pnpm test— 88 tests pass, coverage thresholds met (statements 86.66%, branches 84.32%, functions 87%, lines 86.77%)pnpm typecheck— cleanpnpm lint— clean (8 pre-existing warnings in unrelated files)storageProvideroverride +react-native.config.jsexclusion, confirm noNativeModule: AsyncStorage is nullerror and that events still queue/flush.main.🤖 Generated with Claude Code
Note
Medium Risk
Changes default persistence resolution for all RN customers when AsyncStorage is missing or native-linked incorrectly; wrong error handling could silently drop identity/event persistence without crashing.
Overview
Replaces the top-level AsyncStorage import in
local-storage.tswith a cached lazyrequire, so apps can exclude the native module without crashing at SDK load.LocalStoragenow treats a missing package, load failures (with selectiveconsole.warn), and null native bridge errors as disabled persistence—isEnabled()is false and read/write paths no-op or return undefined, includinggetRawfor identity migration callers.Documents the full AsyncStorage opt-out (override
storageProviderandcookieStorage, autolinking exclusion, RN Web stub/alias patterns) in the package README.Adds unit tests for missing package, broken installs, and native-bridge rejection; wires the RN example app with in-memory storage, autolinking exclusion, expanded Maestro smoke (boot + track/identify), and removes RNCAsyncStorage from the example Podfile.lock.
Reviewed by Cursor Bugbot for commit 36c1d53. Bugbot is set up for automated code reviews on this repo. Configure here.