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

[No QA] Turn off multiple withOnyx eslint rule for now #28624

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Oct 2, 2023

cc @studentofcoding

Details

Until I have a valid solution for using a single withOnyx call, disable this ESLint rule so we don't have to keep adding suppressions everywhere.

Fixed Issues

None

Tests

None

  • Verify that no errors appear in the JS console

Offline tests

None

QA Steps

None

  • 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@tgolen tgolen requested a review from a team as a code owner October 2, 2023 15:43
@tgolen tgolen self-assigned this Oct 2, 2023
@melvin-bot melvin-bot bot requested review from thienlnam and removed request for a team October 2, 2023 15:43
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

@thienlnam 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]

@tgolen tgolen changed the title [No QA] Turn of multiple withOnyx eslint rule for now [No QA] Turn off multiple withOnyx eslint rule for now Oct 3, 2023
@thienlnam thienlnam merged commit a4efd74 into main Oct 4, 2023
14 of 15 checks passed
@thienlnam thienlnam deleted the tgolen-patch-3 branch October 4, 2023 05:31
@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 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.

@studentofcoding
Copy link
Contributor

Noted @tgolen

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Oct 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1298.758 ms → 1480.767 ms (+182.009 ms, +14.0%) 🔴
App start runJsBundle 897.000 ms → 1028.406 ms (+131.406 ms, +14.6%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1298.758 ms
Stdev: 35.921 ms (2.8%)
Runs: 1203.2548249997199 1242.9582490008324 1254.294393999502 1255.1300259996206 1263.644192000851 1266.3508400004357 1270.6452929992229 1275.227927999571 1275.923764999956 1276.0004409998655 1277.3808650001884 1280.5903309993446 1294.736547999084 1295.820304999128 1300.2039700001478 1300.668818000704 1306.5580620002002 1313.0948929991573 1313.1325160004199 1319.8816599994898 1320.6684930007905 1321.013312999159 1321.9119589999318 1322.8219830002636 1324.7891690004617 1326.8611159995198 1327.2612800002098 1336.1145830005407 1336.4501990005374 1354.7353579998016 1383.3831929992884

Current
Mean: 1480.767 ms
Stdev: 32.120 ms (2.2%)
Runs: 1419.2022440005094 1423.0268109999597 1430.8051340002567 1439.2116930000484 1445.9364800006151 1448.40402800031 1456.8796469997615 1460.4221829995513 1462.933015000075 1462.9409839995205 1465.3307620007545 1466.413048999384 1471.3247229997069 1477.8430610001087 1483.601590000093 1484.241220999509 1484.9166759997606 1487.4511900004 1490.50500700064 1491.935523999855 1498.7057240009308 1501.4843460004777 1505.1443579997867 1505.3658509999514 1514.0466770008206 1522.554212000221 1522.96952399984 1529.327616000548 1530.3591699991375 1539.7290659993887
App start runJsBundle Baseline
Mean: 897.000 ms
Stdev: 27.687 ms (3.1%)
Runs: 831 856 859 860 862 869 871 872 872 884 884 889 890 892 897 897 905 907 907 907 910 912 916 916 917 918 922 924 925 937 946 950

Current
Mean: 1028.406 ms
Stdev: 24.363 ms (2.4%)
Runs: 988 995 1000 1002 1005 1005 1009 1009 1010 1010 1011 1015 1017 1017 1019 1022 1024 1027 1034 1036 1036 1036 1042 1043 1048 1050 1054 1057 1061 1063 1068 1096

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 672.630 ms → 719.629 ms (+46.999 ms, +7.0%)
App start nativeLaunch 22.786 ms → 23.893 ms (+1.107 ms, +4.9%)
App start regularAppStart 0.021 ms → 0.019 ms (-0.002 ms, -8.3%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 672.630 ms
Stdev: 36.604 ms (5.4%)
Runs: 614.6464850008488 617.0274659991264 633.68404099904 634.6601979993284 637.0649820007384 637.4216320011765 642.093628000468 644.6357430014759 646.9435229990631 652.4612630009651 653.2871100008488 653.7197270002216 654.2722580004483 656.2742929998785 656.5300300009549 657.1227620001882 665.1354169994593 674.1197109986097 675.347289999947 680.1111249998212 683.4165039993823 687.03886000067 688.4606129992753 690.5303549990058 690.5695399995893 693.4559330008924 715.8512379992753 715.9872650001198 724.4440110009164 739.643595000729 740.4702559988946 767.719076000154

Current
Mean: 719.629 ms
Stdev: 60.114 ms (8.4%)
Runs: 649.6650390010327 659.3181970007718 661.3621020000428 662.7416999991983 663.1877440009266 663.6766360010952 664.2338059991598 664.7530119996518 671.6424560006708 677.6027429997921 679.3506259992719 680.6518970001489 684.059936998412 685.0322670005262 687.3128669988364 690.5484619997442 690.6390790008008 700.0281980000436 712.575276998803 722.4874269999564 724.4794110003859 732.7474360000342 736.3615320008248 739.7492270004004 770.7100430000573 770.9212250001729 773.5555419996381 786.9047449994832 796.6071780007333 798.8680019993335 812.8726810012013 841.8328859992325 891.2617600001395
App start nativeLaunch Baseline
Mean: 22.786 ms
Stdev: 1.655 ms (7.3%)
Runs: 20 21 21 21 21 22 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 24 25 25 25 25 26 27

Current
Mean: 23.893 ms
Stdev: 1.423 ms (6.0%)
Runs: 22 22 22 22 22 23 23 23 23 23 23 23 24 24 24 24 24 24 24 24 25 25 25 25 26 26 27 27
App start regularAppStart Baseline
Mean: 0.021 ms
Stdev: 0.002 ms (7.4%)
Runs: 0.01765899918973446 0.017660001292824745 0.01839200034737587 0.01855500042438507 0.018635999411344528 0.019042998552322388 0.019612999632954597 0.01977499946951866 0.019816000014543533 0.019937999546527863 0.020019998773932457 0.020183000713586807 0.020223001018166542 0.020385999232530594 0.020385999232530594 0.020508000627160072 0.0206300001591444 0.020711001008749008 0.02087399922311306 0.020955000072717667 0.021198999136686325 0.0215659998357296 0.021648000925779343 0.021770000457763672 0.022053999826312065 0.02225799858570099 0.022338999435305595 0.022379999980330467 0.02323400042951107 0.023966001346707344

Current
Mean: 0.019 ms
Stdev: 0.000 ms (2.6%)
Runs: 0.01769999973475933 0.01810700073838234 0.018187999725341797 0.01822900027036667 0.018350999802350998 0.0185139998793602 0.0185139998793602 0.01855500042438507 0.018596000969409943 0.018635999411344528 0.018635999411344528 0.018675999715924263 0.018717000260949135 0.018717000260949135 0.018717000260949135 0.018717000260949135 0.018718000501394272 0.018758000805974007 0.018921000882983208 0.019001999869942665 0.019001999869942665 0.019042998552322388 0.01908300071954727 0.019206000491976738 0.019286999478936195 0.019328000023961067 0.019328000023961067 0.019450001418590546 0.019491000100970268 0.0197759997099638 0.020100999623537064

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

OSBotify commented Oct 5, 2023

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.78-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2023

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2023

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.79-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants