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

Consider performance and memoization/pure components #61

Closed
Ligerx opened this issue Jun 2, 2019 · 6 comments · Fixed by #74
Closed

Consider performance and memoization/pure components #61

Ligerx opened this issue Jun 2, 2019 · 6 comments · Fixed by #74

Comments

@Ligerx
Copy link
Owner

Ligerx commented Jun 2, 2019

Shit's been slow recently. Also if I'm moving everything into selectors, using redux hooks, and removing the top down data passing, it's a good opportunity to look at performance.

There's no consensus if you should just wrap all your components in React.memo or not.
facebook/react#14463
https://twitter.com/dan_abramov/status/1083897065263034368?lang=en
https://twitter.com/tesseralis/status/1083899703262011392

Note: Redux hooks docs recommends using useCallback when passing dispatch to a (memoized) child component.
https://react-redux.js.org/next/api/hooks#examples

Video tutorial for performance optimization
https://www.youtube.com/watch?v=iTrCNz1gRt0

@Ligerx
Copy link
Owner Author

Ligerx commented Jun 2, 2019

While working on PR #58, I'm going to skip out on optimizations and React.memo for now.

@Ligerx
Copy link
Owner Author

Ligerx commented Jun 8, 2019

  • Optimize MangaInfo part 1
    On MangaInfo page, large numbers of chapters cause a ton of lag due to a high number of ChapterListItem and Popover components being rendered. My guess is that the only way to solve for this is to use something like react-window or react-virtualized.
    I do wonder how much of the performance problem is due to using react devtools and not using a production build. With react devtools turned off, a list of 200 chapters is a little more performant, but still noticeably laggy sometimes.

https://material-ui.com/components/lists/#virtualized-list
https://blog.logrocket.com/windowing-wars-react-virtualized-vs-react-window/
https://github.com/bvaughn/react-window
https://github.com/bvaughn/react-virtualized
mui/material-ui#7450

  • Optimize MangaInfo part 2
    Also, double check if flag updates are being eagerly committed to the store. This might account for some of the additional lag I'm seeing.
    Just checked, both library and mangaInfo flags are set optimistically already.

@Ligerx
Copy link
Owner Author

Ligerx commented Jun 8, 2019

  • Optimize filters
    In filters, changing any value will make everything rerender. It seems that just wrapping all the lower level components in React.memo() doesn't fix this. It probably has to do with how I'm updating the filters in the store.
    I wonder if the correct way to fix this is to move all the action dispatching and state fetching down to the individual components.
    Note: if the lowest level components are connected to redux, I should remove the React.memo() wrapper around them

@Ligerx
Copy link
Owner Author

Ligerx commented Jun 8, 2019

I haven't taken a look at Reader yet because I'm hoping to rewrite it soon. Revisit it in the future.

@Ligerx
Copy link
Owner Author

Ligerx commented Jun 8, 2019

Doing more research into react-window and react-virtualized.
I think it's actually better UX to NOT have the content above the chapter list scroll away. So then my requirements is that it has autosizing (fill the remaining window space). I also want to reconsider the visual treatment of the virtualized list after implementing it.

To make the chapter list fill up only the rest of viewport height, I think I'm going to need to wrap all the dom elements on the page with a flex box style. Then the list will have to flex grow to fill.
html, body, and root already seem to be height 100% so this should be straightforward to implement.

@Ligerx
Copy link
Owner Author

Ligerx commented Jun 9, 2019

PR #74 closes this. However, the virtualized list doesn't fully solve the chapter list issue. Each ChapterListItem is still super expensive to render and I'm going to open another issue specifically to explore this.

@Ligerx Ligerx closed this as completed in #74 Jun 9, 2019
Ligerx added a commit that referenced this issue Jun 9, 2019
* Update filters to use style hooks and memo

* Refactoring filters WIP

* More filters updates

* Mostly finish optimizations

* Extract a DynamicFilter component

* Add some comments

* Suppress a warning

* install react-window

* WIP virtualized chapter list

* WIP visual updates to virtualized chapter list

* Fix naming typo

* Implement virtualized chapter list. Not perfect but it works

* Don't render chapters if the list is empty
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 a pull request may close this issue.

1 participant