Skip to content
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

Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32 #13967

Conversation

chrispader
Copy link
Contributor

@chrispader chrispader commented Jan 3, 2023

Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32

Details

In order to migrate react-native-onyx to use react-native-quick-sqlite instead of AsyncStorage on the native applications we'll need to update react-native-onyx to 1.0.32.

Fixed Issues

$ #10793

Tests

  • User should get signed out once he/she updates the app (after updating react-native-onyx to 1.0.32)
  • All flows should still be working after user signs in back again
  1. Sign in
  2. Test all flows on their functionality
  • Verify that no errors appear in the JS console

Offline tests

You can't sign in while you're offline.

QA Steps

Same steps as in "Tests" section

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Not affected by this change, since only native platforms use react-native-quick-sqlite after this update

image
Mobile Web - Chrome

Not affected by this change, since only native platforms use react-native-quick-sqlite after this update

image
Mobile Web - Safari

Not affected by this change, since only native platforms use react-native-quick-sqlite after this update

image
Desktop

Not affected by this change, since only native platforms use react-native-quick-sqlite after this update

Screenshot 2023-01-03 at 15 10 16
iOS

App initial screen after update:

Simulator Screen Shot - iPhone 14 Pro - 2023-01-03 at 14 37 48

Android

App initial screen after update:

android-screenshot

@chrispader chrispader requested review from tgolen and a team as code owners January 3, 2023 22:44
@melvin-bot melvin-bot bot requested review from cristipaval and removed request for a team January 3, 2023 22:45
@melvin-bot
Copy link

melvin-bot bot commented Jan 3, 2023

@cristipaval 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
Copy link
Contributor Author

cc @tgolen, this one should be ready for review

tgolen
tgolen previously approved these changes Jan 3, 2023
@tgolen
Copy link
Contributor

tgolen commented Jan 3, 2023

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web 2023-01-03_15-16-44
Mobile Web - Chrome 2023-01-03_15-25-34
Mobile Web - Safari 2023-01-03_15-17-55
Desktop 2023-01-03_15-19-13
iOS 2023-01-03_15-18-38
Android image

@tgolen tgolen changed the title Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32 [HOLD full-regression-testing] Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32 Jan 3, 2023
cristipaval
cristipaval previously approved these changes Jan 4, 2023
@cristipaval
Copy link
Contributor

@tgolen just for my information: Does Melvin know to manage the fixed issue if it is linked like that?

@tgolen
Copy link
Contributor

tgolen commented Jan 4, 2023

@chrispader No, that format will mess up melvin. It needs to be like this (without the URL markdown):

$ https://github.com/Expensify/App/issues/10793

@tgolen
Copy link
Contributor

tgolen commented Jan 4, 2023

Any idea why the jest tests are failing?

@chrispader
Copy link
Contributor Author

@chrispader No, that format will mess up melvin. It needs to be like this (without the URL markdown):

$ https://github.com/Expensify/App/issues/10793

like this? (edited description)

@chrispader chrispader dismissed stale reviews from cristipaval and tgolen via 25a2eeb January 4, 2023 20:02
@chrispader chrispader force-pushed the @chrispader/migrate-to-sqlite/update-react-native-onyx branch from cfe8326 to 25a2eeb Compare January 4, 2023 20:02
@chrispader
Copy link
Contributor Author

Any idea why the jest tests are failing?

investigating that right now

@chrispader
Copy link
Contributor Author

Fixed the tests. Turned out to be an issue with some other changes that arised with the latest changes since 1.0.29, where Onyx.clear() now takes a parameter and in some tests Onyx.clear was just passed as a callback, rather than running it in a lambda

@chrispader chrispader force-pushed the @chrispader/migrate-to-sqlite/update-react-native-onyx branch from 14fbf91 to d119ccc Compare January 5, 2023 17:52
@chrispader chrispader force-pushed the @chrispader/migrate-to-sqlite/update-react-native-onyx branch from d119ccc to 842391c Compare January 5, 2023 19:13
@OSBotify
Copy link
Contributor

OSBotify commented Jan 6, 2023

@tgolen
Copy link
Contributor

tgolen commented Jan 9, 2023

Looks like testing is 76% complete and no issues have been found yet, so that's a good sign.

@tgolen tgolen changed the title [HOLD full-regression-testing] Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32 Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32 Jan 9, 2023
@tgolen
Copy link
Contributor

tgolen commented Jan 9, 2023

All testing is done and no issues were found!

@tgolen tgolen merged commit 0732a45 into Expensify:main Jan 9, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Jan 9, 2023

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 667.200 ms → 682.660 ms (+15.459 ms, +2.3%)
App start runJsBundle 182.938 ms → 183.594 ms (+0.656 ms, ±0.0%)
App start nativeLaunch 8.844 ms → 9.419 ms (+0.576 ms, +6.5%)
App start regularAppStart 0.014 ms → 0.014 ms (+0.000 ms, +3.3%)
Open Search Page TTI 614.221 ms → 600.680 ms (-13.541 ms, -2.2%)
Show details
Name Duration
App start TTI Baseline
Mean: 667.200 ms
Stdev: 32.418 ms (4.9%)
Runs: 612.6404270008206 614.5367329996079 624.251174999401 628.079346999526 634.8251249995083 636.9422120004892 639.4928920008242 641.9955000001937 646.0005159992725 647.3132700007409 649.321337999776 650.3949439991266 651.6082789991051 656.5005370005965 661.2961260005832 662.5181680005044 672.6199609991163 673.403381999582 678.6945539992303 679.6785170007497 680.9919659998268 681.5918230004609 681.628581000492 686.5159440003335 687.5502640008926 695.0134009998292 705.0425390005112 711.1988910008222 716.6905809994787 734.308313999325 740.5659279990941

Current
Mean: 682.660 ms
Stdev: 31.580 ms (4.6%)
Runs: 631.9479300007224 634.462084999308 638.4810479991138 644.8567239996046 649.8244059998542 653.0241989996284 655.5435400009155 655.9091749992222 657.215168999508 662.8761849999428 663.9558320008218 664.9769139997661 668.2046450003982 675.1978730000556 676.4260579999536 677.980732999742 679.866778999567 681.8141799997538 685.7902520000935 693.0720440000296 696.5010480005294 697.1908849999309 697.448987999931 697.5780599992722 701.0842020008713 710.959760999307 711.2339859995991 720.5211050007492 727.5728110000491 734.431699000299 739.8995019998401 759.2658810000867
App start runJsBundle Baseline
Mean: 182.938 ms
Stdev: 21.832 ms (11.9%)
Runs: 149 156 160 161 161 163 164 165 167 169 169 170 171 174 175 178 180 181 182 185 186 186 193 195 199 202 206 210 210 215 228 244

Current
Mean: 183.594 ms
Stdev: 21.922 ms (11.9%)
Runs: 151 154 157 157 158 162 164 164 167 169 175 176 177 178 181 182 183 183 184 186 186 187 188 188 198 204 205 207 208 225 231 240
App start nativeLaunch Baseline
Mean: 8.844 ms
Stdev: 1.176 ms (13.3%)
Runs: 7 7 7 8 8 8 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 11 12

Current
Mean: 9.419 ms
Stdev: 1.914 ms (20.3%)
Runs: 7 7 8 8 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 10 10 10 11 11 11 11 11 13 14 15
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (9.4%)
Runs: 0.01192300021648407 0.012126000598073006 0.012246999889612198 0.01228799857199192 0.012491000816226006 0.012492001056671143 0.012614000588655472 0.012653999030590057 0.012816999107599258 0.01314299926161766 0.01314300112426281 0.013304999098181725 0.013509001582860947 0.01355000026524067 0.013590000569820404 0.01371300034224987 0.013793999329209328 0.014161000028252602 0.014200998470187187 0.014527000486850739 0.014607999473810196 0.014689000323414803 0.014852000400424004 0.014934001490473747 0.015013998374342918 0.015095999464392662 0.015217998996376991 0.015298999845981598 0.01590999960899353 0.016439000144600868 0.01700800098478794

Current
Mean: 0.014 ms
Stdev: 0.001 ms (7.3%)
Runs: 0.013101998716592789 0.01314300112426281 0.01314300112426281 0.01326499879360199 0.01342800073325634 0.013590998947620392 0.013916000723838806 0.013916000723838806 0.013955999165773392 0.013996999710798264 0.014078998938202858 0.014118999242782593 0.01411999948322773 0.014159999787807465 0.014160001650452614 0.014200998470187187 0.014201000332832336 0.014404000714421272 0.014607999473810196 0.014649000018835068 0.014851998537778854 0.014973999932408333 0.015014998614788055 0.015543999150395393 0.015665998682379723 0.015829000622034073 0.01822900027036667
Open Search Page TTI Baseline
Mean: 614.221 ms
Stdev: 19.994 ms (3.3%)
Runs: 569.1172290015966 572.9490159992129 578.4961759988219 589.5607910007238 589.5973710007966 600.0894780009985 603.9997969996184 605.261800000444 606.4212650004774 608.3485920000821 608.4805100001395 608.5570070017129 608.7952479999512 609.0995279997587 610.4024660009891 611.2230230011046 611.7595220003277 617.5784109998494 618.6051839999855 621.472005000338 625.6329350005835 625.984253000468 626.5262859985232 626.7708749994636 627.2987469993532 631.3022869993001 632.4355069994926 632.8553470000625 634.2693679984659 637.2677009999752 640.766235999763 664.1634519994259

Current
Mean: 600.680 ms
Stdev: 21.722 ms (3.6%)
Runs: 550.7376309987158 556.7401940003037 576.1311850007623 578.0327150002122 579.1015629991889 584.6265459991992 585.1238199993968 586.0609540008008 586.808960000053 589.011921999976 590.1386719997972 590.8526619989425 593.1507980003953 593.1909999996424 593.7937430012971 593.9902340006083 599.4412840008736 600.1981609985232 601.644369000569 604.1639409996569 604.1919759996235 608.0291750002652 609.6536459997296 615.2942719999701 615.4435230009258 618.6165770012885 618.9733480010182 619.1687420010567 624.5131429992616 631.270060999319 637.1044110003859 643.5668950006366 643.6829020008445

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @tgolen in version: 1.2.52-0 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants