make async storage optional for react-native - #2781
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReact Native storage now treats AsyncStorage as optional, loads it dynamically when available, and uses a fallback that reports a configuration error when it is absent. Installation guidance and documentation were updated, and the shared version changed to v1.0.51. ChangesReact Native storage fallback
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant StorageModule
participant AsyncStorage
participant MissingStore
StorageModule->>AsyncStorage: Attempt optional require
alt AsyncStorage available
AsyncStorage-->>StorageModule: Select AsyncStorageStore
else AsyncStorage unavailable
StorageModule->>MissingStore: Select fallback class
MissingStore-->>StorageModule: Throw configuration error on construction
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/packages/react-native/src/Storage.native.ts (1)
8-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSafely resolve the
AsyncStoragemodule export.Consider using the
mod.default ?? modpattern here to match how you're resolving@instantdb/react-native-mmkvand@instantdb/expo-sqliteon lines 94 and 98. Depending on a user's specific bundler or module resolution configuration,requiremight return the module object directly without a.defaultproperty. If this happens,AsyncStoragewill be set toundefined, which silently triggers the fallback and throws theMissingStoreerror even when the package is correctly installed.♻️ Proposed refactor
let AsyncStorage: any = null; try { - AsyncStorage = require('`@react-native-async-storage/async-storage`').default; + const mod = require('`@react-native-async-storage/async-storage`'); + AsyncStorage = mod.default ?? mod; } catch {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/packages/react-native/src/Storage.native.ts` around lines 8 - 11, Update the AsyncStorage initialization in the module-loading try block to retain the required module object when no default export exists, using the same default-or-module resolution pattern as the react-native-mmkv and expo-sqlite loaders. Preserve the existing fallback behavior when the require itself fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/packages/react-native/src/Storage.native.ts`:
- Around line 8-11: Update the AsyncStorage initialization in the module-loading
try block to retain the required module object when no default export exists,
using the same default-or-module resolution pattern as the react-native-mmkv and
expo-sqlite loaders. Preserve the existing fallback behavior when the require
itself fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4ebe5ee-21bd-4ce5-8051-e11fe050bd7c
📒 Files selected for processing (5)
client/packages/react-native-mmkv/README.mdclient/packages/react-native/package.jsonclient/packages/react-native/src/Storage.native.tsclient/packages/version/src/version.tsclient/www/app/docs/start-rn/page.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/packages/react-native/src/Storage.native.ts (1)
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd isolated coverage for both default-store branches.
Mock the optional dependency before importing the module and verify that the default export is
AsyncStorageStorewhen present andMissingStorewhen absent; also coverinitwith an explicit customStore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/packages/react-native/src/Storage.native.ts` at line 85, Add isolated tests for the default export in Storage.native, mocking the optional AsyncStorage dependency before each module import to verify AsyncStorageStore when available and MissingStore when unavailable. Also test init with an explicitly supplied custom Store, preserving the existing default-store behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/packages/react-native/src/Storage.native.ts`:
- Around line 5-10: Update the AsyncStorage loading fallback in
Storage.native.ts so it only treats a genuinely absent optional module as
MissingStore; rethrow other errors from the require, including native, linking,
or version failures. Preserve the direct require placement inside the try block
so Metro continues recognizing AsyncStorage as optional, and keep custom Store
handling unchanged.
---
Nitpick comments:
In `@client/packages/react-native/src/Storage.native.ts`:
- Line 85: Add isolated tests for the default export in Storage.native, mocking
the optional AsyncStorage dependency before each module import to verify
AsyncStorageStore when available and MissingStore when unavailable. Also test
init with an explicitly supplied custom Store, preserving the existing
default-store behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fa0f969-4657-4ab3-96e7-ed3dbb9805ec
📒 Files selected for processing (2)
client/packages/react-native/src/Storage.native.tsclient/packages/version/src/version.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/packages/version/src/version.ts
Which store an app persists to shouldn't depend on what happens to be in node_modules: installing async storage for an unrelated library would silently flip an auto-detected mmkv app back to AsyncStorage, stranding any unsynced offline mutations. So if async storage isn't installed, init now throws with instructions instead of probing for wrapper packages. Wrapper stores keep working by passing Store to init, which is the documented path. Also bumps the version to v1.0.51 since v1.0.50 already shipped with the expo-sqlite adapter.
0d3b089 to
e4b390f
Compare
The optional require swallows every error, so an installed async-storage whose native side isn't linked would produce a misleading "install async-storage" message at init. Rethrowing isn't an option (it would crash apps that pass a custom Store at import time), and Metro gives no reliable way to tell "absent" from "broken", so instead we capture the require error and append it to the MissingStore message. A half-linked install now shows async-storage's own error with its rebuild instructions.
|
Great work @pvinis . I like this PR in general. I removed the part where we auto-detect the different storage families. Main reason was about predictability: i.e Imagine installing @instantdb/react-native-mmkv. But then, you also install react-native-async-storage for something else. You may unexpectedly switch to react-native-async-storage. This keeps react-native-async storage the only default, but keeps the logic to let you omit it if you are using a custom store. |
|
thanks for taking a look! fair. my thought was that maybe we add a thing in config for which one to use. would that maybe be better? so if I just have mmkv and don't want asyncstore, I set it up to use mmkv? |
2 participants
right now
@instantdb/react-nativehard-imports@react-native-async-storage/async-storageat the top ofStorage.native.ts, so apps that pass a customStoretoinit(mmkv from #2219, expo-sqlite from #2780) still have to install async storage just to make the bundle resolve, andexpo doctorerrors on the missing peer dep.this makes the default store lazy and optional:
@instantdb/react-native-mmkv, then@instantdb/expo-sqlite, if one of those is installed. installing a wrapper package is enough, no need to passStoreexplicitly (though you still can).Store:initthrows with instructions, instead of the bundler failing on a module it can't resolve.the requires sit directly inside
tryblocks, which metro treats as optional dependencies (transformer.allowOptionalDependencies, enabled by default in both@expo/metro-configand@react-native/metro-config), so bundling no longer fails when async storage is absent. the peer dep is also marked optional viapeerDependenciesMeta, which is what fixes theexpo doctorerror. the async storage store is still exported asAsyncStorageStoreif anyone wants to force it.one caveat to flag: on a metro config that does not enable
allowOptionalDependencies(vanilla metro without the expo or rn presets), the two wrapper requires would now fail to resolve for apps that don't have them installed. both standard configs enable it, so this should be a non-issue in practice, but mentioning it in case you'd rather guard differently.verified the resolution logic in node with stubbed modules (all four combinations pass: async storage wins when present, mmkv then expo-sqlite as fallbacks, helpful error when nothing is there), and build +
test:cipass. also dropped async storage from the mmkv readme install line and added a note to the rn docs page. no lockfile change needed.