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

Centralize Onyx subscriptions in ReportScreen #4594

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Aug 12, 2021

Details

This PR should improve report switching time a bit since withOnyx() HOCs are fairly expensive to initialize and we can reduce the number of keys we subscribe to by passing some props to the ReportScreen children instead of creating duplicate subscriptions.

Fixed Issues

Not really related to any particular issue - we are trying to improve chat switch metrics and there's a tracking issue here:

#4022

Tests

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Aug 12, 2021
@marcaaron marcaaron changed the title [WIP] More ReportScreen refactors. Move more critical keys into context. [WIP] Consolidate ReportScreen into single component to share Onyx subscriptions Aug 12, 2021
Fix the context methods

more report screen organizing

create more context helpers

remove bad merged code

remove unusued imports and fix style

undo context changes

remove unused line

add missing session
@marcaaron marcaaron force-pushed the marcaaron-reduceReportSubscriptions branch from 5c7635d to fd2f824 Compare August 12, 2021 01:54
@marcaaron marcaaron changed the title [WIP] Consolidate ReportScreen into single component to share Onyx subscriptions [WIP] Centralize Onyx subscriptions in ReportScreen Aug 12, 2021
@@ -26,7 +26,7 @@ export default (onyxKeyName) => {
},
})(Provider);

const withOnyxKey = ({propName = onyxKeyName, transformValue = () => {}} = {}) => (WrappedComponent) => {
const withOnyxKey = ({propName = onyxKeyName, transformValue} = {}) => (WrappedComponent) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated but fixes an issue where we only return the value if the function is undefined but by defaulting to an anonymous function that never happens.

@marcaaron marcaaron changed the title [WIP] Centralize Onyx subscriptions in ReportScreen Centralize Onyx subscriptions in ReportScreen Aug 12, 2021
@marcaaron marcaaron marked this pull request as ready for review August 12, 2021 02:05
@marcaaron marcaaron requested a review from a team as a code owner August 12, 2021 02:05
@MelvinBot MelvinBot requested review from stitesExpensify and removed request for a team August 12, 2021 02:05
@marcaaron marcaaron merged commit 5acd236 into main Aug 13, 2021
@marcaaron marcaaron deleted the marcaaron-reduceReportSubscriptions branch August 13, 2021 20:00
@OSBotify
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.0.85-10 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.0.86-11 🚀

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants