Switch to React #729
Switch to React #729
Conversation
Work left to do
|
|
We should also use React in a more idiomatic way. I ported it with minimum changes, but later on we should convert function calls that return a vdom like |
dcposch
force-pushed
the
dc/perf
branch
2 times, most recently
from
b7a60e3 to
d2e3cfd
Compare
Speed test
The timeline view shows FPS and breaks down where time is being spent. hyperxCan't really scroll. See #725 ReactWith React in production mode (extra checks and warnings disabled with NODE_ENV=production, which is what our users see): This is better, but unfortunately the scrolling is still choppy. First, 70ms is a lot longer than 16ms, so you are freezing for a few frames (compared to 60fps) every time you call update(). Second, even when the DOM is not updating at all, Chrome can't scroll it smoothly. Below, the orange parts correspond to time spent in JS (our code and React). The purple parts are time spent in Chrome rendering frames of scrolling animation while the DOM isn't changing.
In short it's better now, but still not smooth. IMO this PR is a good first step. Next we'll have to make the DOM more lightweight (eg by capping the number of file rows we show). |
|
The dom is just SLOW I saved the torrent list view as static HTML:
A few stats:
And this is plain HTML + CSS, no Javascript of any kind. Anyway despite that, when mousing over the list, the As you can see below it runs at about 10FPS.
|
Turns out this is huge. For some inexplicable reason, it improves hover and scroll in the torrent detail view, for a torrent with 350 files, from ~10FPS to ~60FPS.
|
@dcposch Here's an idea: remove the background image. It's hugely stretched. I bet that's causing this. |
| @@ -129,8 +129,6 @@ module.exports = class TorrentController { | |||
| // files which are completed after a video starts to play to be added | |||
| // dynamically to the list of subtitles. | |||
| // checkForSubtitles() | |||
|
|
|||
| dispatch('update') | |||
There was a problem hiding this comment.
It was causing double update()s.
| @@ -14,9 +15,11 @@ | |||
| height: 140px; | |||
| } | |||
| </style> | |||
| <script> | |||
| require('../build/renderer/webtorrent.js') | |||
There was a problem hiding this comment.
Same question: why not <script async ...>?
There was a problem hiding this comment.
Doing that breaks require(). If we want <script ...> to work, we either have to:
- Write ugly things like
require('../build/renderer/<etc>')in the actual source files - Move
main.htmlto be next tomain.js
Doing it this way lets us separate src/ for JS sources only and static/ for HTML, CSS, etc
|
Hey @dcposch, this is nice work! 👍 👍 👍 Never used React before, but curious to see how this goes. It looks pretty nice so far. I left lots of comments and questions inline. Once comments are addressed, let's merge this! I think we shouldn't release for a few days after this is merged since I wasn't able to compare line-by-line for some of the files. Some of the file diffs just show every line removed, then every line added. :( So let's wait a few days for any bugs to get shaken out. Maybe React isn't so bad... |
|
@feross I tried removing the background image, but that wasn't the cause of the slowness. |
|
👍 |
|
And scrolling thru the 350 files in Adventures of Huckleberry Finn is fast now! |
|
👍 👍 👍 |
Add window.client = the WebTorrent client
The goal is to make the app faster and smoother. As a bonus, React is more standard and more actively maintained than hyperx/virtual-dom/main-loop.
See #725 for motivation
@feross