chore(deps): Update NitroSQLite to 9.8.1 - #101437
chrispader wants to merge 6 commits into
Conversation
|
|
chrispader
changed the title
chrispader
changed the title
|
@MelvinBot please create an Expensify/App issue to track updating NitroSQLite from 9.6.0 to the latest upstream code, packaged in this PR as an unreleased 9.8.0 snapshot at Please include the main reasons for the update:
Answered by Codex, instructed by Chris. |
|
Created chore(deps): Update NitroSQLite from 9.6.0 to 9.8.0 and assigned it to The issue tracks the dependency update itself: the I also updated this PR's |
chrispader
changed the title
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
chrispader
changed the title
|
@MelvinBot can you please request review from @NicolasBonet 🙌🏼 |
|
Requested a review from Current reviewers on this PR: |
ec26355 to
719b37c
Compare
|
@ikevin127 could you please follow the steps in here: #96531 (Tests A, B and C)? 🙏 |
chrispader
changed the title
|
I've just noticed that i've unnecessarily bumped the peer dependency range for NitroModules in the last NitroSQLite release, i'm going to lower that again in I'll ping @ikevin127 once i've updated the PR! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
I'll hold on reviewing until you give the green light 🙌 |
chrispader
changed the title
chrispader
changed the title
|
@ikevin127 the PR is ready now! |
chrispader
changed the title
| "react-native-nitro-fetch": "1.5.4", | ||
| "react-native-nitro-modules": "0.36.3", | ||
| "react-native-nitro-sqlite": "9.6.0", | ||
| "react-native-nitro-sqlite": "9.8.1", |
There was a problem hiding this comment.
🔴 package.json:203 (behaviour lands in src/libs/ExportOnyxState/index.native.ts:19)
Settings → Troubleshoot → Export Onyx state breaks silently on iOS and Android.
9.8.1's cpp/operations.cpp is byte-identical to 9.8.0:
void sqliteOpenDb(const std::string& dbName, const std::string& docPath) {
std::lock_guard lifecycleLock(dbLifecycleMutex);
{
std::lock_guard lock(dbMapMutex);
if (dbMap.contains(dbName)) {
throw NitroSQLiteException::DatabaseAlreadyOpen(dbName);
}
}and the JS layer still rejects it before the native call is even reached:
// 9.8.1 lib/module/operations/session.js
export function open(options) {
openDatabaseQueue(options.name); // throws "Database OnyxDB is already open."We open OnyxDB a second time here, while Onyx's SQLiteProvider already holds it:
// src/libs/ExportOnyxState/index.native.ts
onyxDb = open({name: CONST.DEFAULT_DB_NAME});9.6.0 tolerated this because sqliteOpenDb just did dbMap[dbName] = db;. The throw happens inside the new Promise executor in readFromOnyxDatabase, and TroubleshootPage.tsx:92 has no .catch, so the user taps Export Onyx state and gets nothing at all: no share sheet, no error, no log. That is our primary tool for debugging user reports.
This is the exact blocker the deleted patches/react-native-nitro-sqlite/details.md recorded:
its new per-database queue breaks second opens of the same database (used by
src/libs/ExportOnyxState/index.native.ts)
The SQLITE_THREADSAFE=0 half of that note is genuinely fixed (9.8.1's podspec still defaults threadSafe to true). The second-open half is not, and the note documenting it is being deleted.
Fix belongs in src/libs/ExportOnyxState/index.native.ts: reuse Onyx's existing connection instead of opening a new one, or read through Onyx's own API. At minimum add a .catch in TroubleshootPage.tsx so it fails loudly.
Please also add "Troubleshoot → Export Onyx state, verify the share sheet opens with a populated dump" to the Tests section and run it on both platforms.
|
|
||
| // NitroSQLite now migrates each database when it opens, using the caller's database name. | ||
| const nitroSQLiteContent = fs.readFileSync(path.resolve(__dirname, '../../node_modules/react-native-nitro-sqlite/cpp/hybridObjects/HybridNitroSQLite.cpp'), 'utf8'); | ||
| expect(nitroSQLiteContent).toContain('return migrateDatabase(dbName,'); |
There was a problem hiding this comment.
🟠 tests/unit/MoveFilesOutOfDocumentsTest.ts:174
I confirmed the string is present in 9.8.1 (cpp/hybridObjects/HybridNitroSQLite.cpp:86), so it passes today. Two problems remain. It is a raw substring match on upstream C++, so any reformat upstream breaks our build for no real reason. And it does not assert what the it() title claims, there is no longer anything tying CONST.DEFAULT_DB_NAME to the migration.
Meanwhile the thing that can actually regress silently is unguarded. ios/OnLoad.mm falls back without failing:
if (databaseLocation != nil && ![databaseLocation isEqualToString:@"Documents"]) {
NSLog(@"Invalid RNNitroSQLite_DatabaseLocation value provided (%@). ... Falling back to \"Documents\".", databaseLocation);
}A typo, or someone dropping the key in a future Info.plist edit, puts OnyxDB straight back into the user-visible Documents folder, which is the bug #96531 fixed, and nothing in CI notices. Swap the C++ grep for something that guards what we own:
it('keeps the iOS database location opted into Application Support', () => {
const infoPlist = fs.readFileSync(path.resolve(__dirname, '../../ios/NewExpensify/Info.plist'), 'utf8');
// NitroSQLite silently falls back to the user-visible Documents directory when this key is
// missing or misspelled, which would put OnyxDB back in the iOS Files app.
expect(infoPlist).toMatch(/<key>RNNitroSQLite_DatabaseLocation<\/key>\s*<string>ApplicationSupport<\/string>/);
});
| "react-native-nitro-fetch": "1.5.4", | ||
| "react-native-nitro-modules": "0.36.3", | ||
| "react-native-nitro-sqlite": "9.6.0", | ||
| "react-native-nitro-sqlite": "9.8.1", |
There was a problem hiding this comment.
🟠 package.json:203
9.8.1 still routes every async op through a per-database JS queue, one at a time with a setImmediate hop between each:
return queueOperationAsync(dbName, () => executeAsyncNative(dbName, query, params));Onyx fans out and expects overlap:
return Promise.all(keyChunks.map((keyChunk) => provider.store.executeAsync(command, keyChunk)))On a High Traffic account that is many chunks now running strictly sequentially instead of concurrently on the native pool. Please post a TTI comparison against main with a High Traffic account on a real low-end device, both platforms, before this merges.
"Send a message and relaunch" will not surface it.
| "react-native-nitro-fetch": "1.5.4", | ||
| "react-native-nitro-modules": "0.36.3", | ||
| "react-native-nitro-sqlite": "9.6.0", | ||
| "react-native-nitro-sqlite": "9.8.1", |
There was a problem hiding this comment.
🟡 package.json:203 (new, from the 9.8.1 respin)
9.8.1's shipped nitrogen/generated/ C++ was generated by nitrogen 0.37.1 (devDependencies.nitrogen: "0.37.1"), while this PR keeps react-native-nitro-modules pinned at 0.36.3.
The peer range that would have caught a mismatch was hand-relaxed from >=0.37.1 to >=0.35.0 specifically to allow this, so it is now a declaration rather than a check.
I did verify every NitroModules header the generated and hand-written C++ includes exists in the installed 0.36.3:
AnyMapUtils.hpp, ArrayBuffer.hpp, ArrayBufferHolder.hpp, DateToChronoDate.hpp,
DefaultConstructableObject.hpp, HybridObject.hpp, HybridObjectRegistry.hpp,
JHybridObject.hpp, JSIConverter.hpp, JSIHelpers.hpp, NitroDefines.hpp,
Null.hpp, Promise.hpp, PropNameIDCache.hpp, RuntimeError.hpp
none missing, so it looks buildable. That is not proof though, signatures inside those headers can still have moved between 0.36.3 and 0.37.1. The only thing that settles it is an actual native build.
The HybridApp bot already asked for one on this PR, so please run AdHoc builds for both iOS and Android (standalone and hybrid) and link them here.
| "react-native-nitro-fetch": "1.5.4", | ||
| "react-native-nitro-modules": "0.36.3", | ||
| "react-native-nitro-sqlite": "9.6.0", | ||
| "react-native-nitro-sqlite": "9.8.1", |
There was a problem hiding this comment.
🟢 package.json:203
9.8.1's podspec still picks build defaults when we say nothing:
app_config = app_package.fetch("nitroSQLite", {})
thread_safe_value = app_config.fetch("threadSafe", true)
performance_mode = app_config.fetch("performanceMode", true)performanceMode defaulting to true newly enables -DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_DEFAULT_WAL_SYNCHRONOUS=1 -DSQLITE_OMIT_SHARED_CACHE=1 ... on the iOS pod, and Android gets none of it (it reads rootProject.properties['nitroSqliteFlags'], which we do not set).
Two platforms compiling SQLite differently, off defaults we never chose. Suggest pinning them so the build is reproducible and the choice is on the record:
"nitroSQLite": {
"threadSafe": true,
"performanceMode": true
}
| <key>RNNitroSQLite_DatabaseLocation</key> | ||
| <string>ApplicationSupport</string> |
There was a problem hiding this comment.
🟢 ios/NewExpensify/Info.plist:98
Correct. Key and value match 9.8.1's ios/OnLoad.mm, and the companion PR sets the same pair. Neither plist sets RNNitroSQLite_AppGroup, which matters because the app-group branch returns before RNNitroSQLite_DatabaseLocation is ever read.
Worth a comment in the plist or a note on the issue so nobody adds an App Group later and quietly reverts the Files-app fix.
StatusSolved: scope creep, Remaining before I can approve:
@chrispader Holding off on running Tests A/B/C until the Export Onyx state fix lands, since I would have to re-run them afterwards anyway. Short version: the respin fixed everything that was noise (scope creep, lockfile churn, pbxproj, patch rename), and I confirmed 9.8.0 → 9.8.1 is purely the peer-range relax with zero source changes. The 🔴 ExportOnyxState regression is therefore completely untouched and still blocks, along with the test assertion, the serialization perf question, and a new 🟡 about 9.8.1's nitrogen-0.37.1 codegen running against pinned NitroModules 0.36.3 (headers all check out, but it needs a real build). |
4 participants
NitroSQLite 9.6.0 still needs local fixes for rollback errors and iOS database storage. The published 9.8.1 release includes those fixes and accepts the app's existing NitroModules 0.36.3. This PR upgrades SQLite and removes both local SQLite patches without changing NitroModules, NitroFetch, or Nitrogen. NewDot and the companion HybridApp PR opt into the upstream Application Support location in their respective Info.plist files.
@NicolasBonet
Explanation of Change
The published NitroSQLite 9.8.1 package replaces the temporary archive built from an upstream commit. The dependency now resolves from the package registry, so the vendored archive and its provenance note are removed.
The upstream batch executor preserves the original error when rollback also fails. Its per-database migration replaces the old OnyxDB-specific patch. NewDot and HybridApp each set
RNNitroSQLite_DatabaseLocationtoApplicationSupportin their host Info.plist, so NitroSQLite needs no local patch. NitroSQLite 9.8.1 accepts NitroModules 0.36.3, allowing this change to retain NitroFetch 1.5.4, Nitrogen 0.36.3, and the existing Android certificate-pinning patch. A separate follow-up PR upgrades the Nitro runtime and its affected packages.ios/Podfile.lockchanges only the RNNitroSQLite version and checksum. The companion HybridApp lockfile pins NitroSQLite 9.8.1 while retaining NitroModules 0.36.3 and NitroFetch 1.5.4.Fixed Issues
$ #101448
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14129
No separate approved proposal applies to this dependency update.
Tests
Manual device verification is pending.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
No screenshots or videos are included because this dependency and plist change has no visible UI.