fossasia / visdom

A flexible tool for creating, organizing, and sharing visualizations of live, rich data. Supports Torch and Numpy.
Apache License 2.0
9.93k stars 1.13k forks source link

[Big change] Migrate main.js to Functional React #900

Closed da-h closed 1 year ago

da-h commented 1 year ago

Description

This PR follows up on #856 and migrates the remaining component (App) to functional React. As a side effect, this PR also fixes all the linting errors related to JavaScript files.

Motivation and Context

The goal is again to modernize the code base to enable better long-term maintainability.

For me, this one has been the most challenging PR so far and it is my third attempt at solving the task. This is probably due to

  1. my personal requirement of trying to keep the PR as reviewable as possible. To accomplish this I have tried to keep all code blocks and functions in the same respective position. Unfortunately, the changes required for the migration are still numerous.
  2. some very specific timing-requirements of redraws for the application to work properly. In the previous code, there have been some comments noticing "non-conventional" React style and I think that these and some other design decisions did not fit well with "hook based react/functional react" anymore. Solving these timing / redraw issues would have required a complete restructuring of many parts of the component, which would have been practically a complete rewrite of the code. This would have clashed with the former requirement.

Thus, I went with a 1:1 translation that is a bit easier to review (the first commit). The PR can be compared with the previous code in a side-by-side manner. The drawback is that the many tricks from the old code (i.e. the "non-conventional" React parts) needed to be retained/reimplemented for the new style. (See the small commits after the huge first one).

Future follow-up PR ideas: After this PR, tidying up the code base further should be possible in smaller PRs again. For instance, I'd suggest to

A final note on the two additional test changes:

How Has This Been Tested?

As I've said, this is my third attempt to finish this PR while making all tests succeed. The new implementation may create new bugs that have not been present before, however, with the testing in place I am confident that almost all features work just as before the PR.

Just to be sure, I've also 10-fold triggered the PR against all tests without any errors on my fork.

Screenshots (if appropriate):

-

Types of changes

Checklist:

JackUrb commented 1 year ago

trying to keep the PR as reviewable as possible

Thanks for taking the extra effort for this - it definitely made it easier for me to check over what was going on. All-in-all your "Future follow-up PR ideas" are exactly what I'd imagine the remaining vision is to shore up the frontend code into a truly clean state.

Maybe not yet? I am confident in the changes I've made, still I'd test the new version for at least a week or two before releasing it to the public. ;)

Honestly with all your DevX, stability, and clarity improvements I think that v1.0.0 could be on the horizon :)

da-h commented 1 year ago

Rebased onto master (including the polling-tests now).

Polling did not work properly with this PR, so I also adapted the polling class to match the new code in a new commit. (I have to admit, I forgot about that). :sweat_smile:

PS: Unfortunately, github does not help very much in showing only these changes. Sorry about possibly complicating reviewing because of the rebase.

da-h commented 1 year ago

Polling-based initial connection is a bit delayed, thus the "manual server reconnect" cypress test had to be adapted to wait a bit for the initial "online" status to appear. I used the opportunity to also add multiple fast user-reconnects to the test.

da-h commented 1 year ago

Honestly with all your DevX, stability, and clarity improvements I think that v1.0.0 could be on the horizon :)

:100: x :heart: Sure! I'd suggest to publish the major release after the proposed changes from here and #898 are done & in-place, possibly including also moving the server to asyncio (?). This should provide a clean code base for anyone to start new improvements on.

da-h commented 1 year ago

No stress, good things take time. :)