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

A lot of rendering refactors related to libraries (and a few extras) #1267

Merged
merged 10 commits into from
May 17, 2022

Conversation

arielj
Copy link
Collaborator

@arielj arielj commented May 3, 2022

This PR includes a lot of refactoring to improve the calculations done in each render:

  • moved all the filter and sort logic from GlobalState to Library (we were doing all the filtering in every render, and even when the library was not shown) and memoize it so it's only calculated when a dependency changes
  • simplified the filter and sort logic to only filter/sort the currently displayed library (we where doing all that for both library every time)
  • moved the numberOfGames calculation from GlobalState to Header (same as before, calculated in every render, even when not needed) and memoize it
  • load current language from browser's storage as soon as possible (removes an issue where english was shown while the component was rendering the first time)
  • memoize the calculation of favourites (we were recalculating them in every render of the library when showing them)
  • set initial state from localstorage to prevent flickering with the wrong state during the initial render
  • move the fetching of recent games into the componentDidMount callback and store it in the state (we were doing that calculation in every render of the global state)
  • prevent refreshing the library while the library is refreshing

I'll add more comments on the diff of the PR


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@@ -143,7 +143,6 @@ export class LegendaryLibrary {

if (fullRefresh) {
try {
logInfo('Refreshing Epic Games...', LogPrefix.Legendary)
Copy link
Collaborator Author

@arielj arielj May 3, 2022

Choose a reason for hiding this comment

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

The refresh function called right after this line also prints this same string to the logs, it gave the impression that the epic library was getting refreshed twice when it didn't.

@@ -32,7 +34,7 @@ i18next
interpolation: {
escapeValue: false
},
lng: 'en',
lng: storage.getItem('language') || 'en',
Copy link
Collaborator Author

@arielj arielj May 3, 2022

Choose a reason for hiding this comment

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

when selecting a language other than en, the app was loaded in english at boot for a moment and then changed to the user's language, which looked glitchy and triggered an extra re-render

@@ -184,9 +281,9 @@ export default function Library(): JSX.Element {

<h3 className="libraryHeader">{titleWithIcons()}</h3>

{refreshing && <UpdateComponent inline />}
{refreshing && !refreshingInTheBackground && <UpdateComponent inline />}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this refreshingInTheBackground to be able to differenciate when the library was being refreshed in the background and in the foreground

@@ -79,7 +81,7 @@ export class GlobalState extends PureComponent<Props> {
return games
}
state: StateProps = {
category: 'epic',
category: storage.getItem('category') || 'epic',
Copy link
Collaborator Author

@arielj arielj May 3, 2022

Choose a reason for hiding this comment

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

we can load all these local storage data here instead of in the componentDidMount callback

with the previous code, if you close heroic when in the gog library, when you open heroic it will open it in the epic library for a moment and then change to gog, which looked glitchy and triggered an extra re-render

@arielj arielj changed the title A lot of rendering refactors related to libraries A lot of rendering refactors related to libraries (and a few extras) May 3, 2022
@flavioislima
Copy link
Member

This is pretty good for performance. As much stuff we can remove from GlobalState the better. Especially to refactor it as a functional component.

@arielj arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label May 7, 2022
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

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

Looks good! Will take some time to test it later.

@flavioislima
Copy link
Member

Tested here and looks good.
Ready to Merge ⚔️

@flavioislima flavioislima added pr:ready-to-merge This PR is fully ready for merge. and removed pr:ready-for-review Feature-complete, ready for the grind! :P labels May 11, 2022
@arielj
Copy link
Collaborator Author

arielj commented May 12, 2022

just to confirm, can I merge this? I know you want to make a new release soon and maybe you prefer to leave this to after the release since it's a refactor, I'm happy to get this merged but I understand if you prefer to wait

@flavioislima
Copy link
Member

Yes, perhaps we should wait for the release then and then put this on on a minor release.
Btw, I just found a small bug: the number of games is not being updated based on the Hidden status.

@flavioislima flavioislima added pr:testing This PR is in testing, don't merge. and removed pr:ready-to-merge This PR is fully ready for merge. labels May 12, 2022
@arielj
Copy link
Collaborator Author

arielj commented May 12, 2022

Yes, perhaps we should wait for the release then and then put this on on a minor release. Btw, I just found a small bug: the number of games is not being updated based on the Hidden status.

thanks, just pushed a fix for this, but let's wait for after the release and then this can be tested more as part of the next betas 👍, I plan to do other refactors too

@flavioislima flavioislima added pr:ready-to-merge This PR is fully ready for merge. and removed pr:testing This PR is in testing, don't merge. labels May 17, 2022
@flavioislima
Copy link
Member

I think this one can be merged :)

@arielj
Copy link
Collaborator Author

arielj commented May 17, 2022

merging this then, so I can see if it adds conflicts to other stuff

@arielj arielj merged commit b75e515 into main May 17, 2022
@arielj arielj deleted the refactor/optimize-library-refresh branch May 17, 2022 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-to-merge This PR is fully ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants