-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD on Onyx#602] Migrate to react-native-nitro-sqlite
in E/App and Onyx
#53149
base: main
Are you sure you want to change the base?
[HOLD on Onyx#602] Migrate to react-native-nitro-sqlite
in E/App and Onyx
#53149
Conversation
@DylanDylann 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 Can you update the |
@dominictb done! |
I only had this problem on this branch but |
Whoops. Seems like a bug with XCode 16.2 mrousavy/nitro#422. I managed to run it after manually changing the minimum deployment target of |
@dominictb As Marc explained in this comment, the problem is an issue in Swift (and therefore in Xcode) itself and it only happens on iOS Simulator. Since this is an issue in Swift, we cannot fix this in Nitro and/or NitroSQLite directly, because it would mean we need to bump the iOS minimum deployment target, which would cause problems with all sorts of other project that use these libraries. I'd suggest to either
I think holding this PR until the underlying Swift issue is fixed is still the most valid choice though, except if we don't care about iOS version |
Let's see what @mountiny thinks about the above idea |
Hmm that is a bummer. I agree that downgrading for all developers is tough thing to manage and also bumping the minimum version is not that easy so I would opt to hold for now |
@mountiny
Explanation of Change
Uses
react-native-nitro-sqlite
instead ofreact-native-quick-sqlite
and updates Onyx to a version that also uses NitroSQLite.Fixed Issues
$ #53063
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop