Infer panel count in App.js from number of default threshold values

[joshhead]

The high level goal here is to allow the React app to accommodate an unknown number of panels without code changes or rebuilding.

I tried to focus on that in this branch and not make too many unrelated changes. However, I did still make some opinionated changes.

I moved some module-level state such as kCurValues, kCurThresholds, and ws into hooks and refs. This has two benefits IMO.

First, the server can shut down and come back with a different number of sensors configured, and the app will handle this gracefully without a page reload. This is not a super important feature, but it demonstrates the second benefit, which is that more resources are managed by React and so their lifetime is well defined including how to clean them up without reloading the page.

I like this style better, but it is more complicated in some ways. There is definitely a simplicity to cleaning up state by reloading the whole page.

I tried to adopt a similar change without refactoring connect(). I think it works okay but I wasn't as happy with it. https://github.com/teejusb/fsr/compare/b89e5228b...joshhead:js-detect-panel-count-minimal?expand=1

[joshhead]

@teejusb I addressed the comments and made a couple additional changes. For the exhaustive deps on ValueMonitor, its dependencies are not causing excessive renders at this point (or any re-renders as far as I can see). IMHO ignoring the exhaustive deps warning tends to cause more problems than it solves, but I can rebase to drop that last commit if you want it left alone.

[teejusb]

@teejusb I addressed the comments and made a couple additional changes. For the exhaustive deps on ValueMonitor, its dependencies are not causing excessive renders at this point (or any re-renders as far as I can see). IMHO ignoring the exhaustive deps warning tends to cause more problems than it solves, but I can rebase to drop that last commit if you want it left alone.

I think my original reasoning for not having the dependencies was because I thought the rendering was handled by requestAnimationFrame instead of by React. In my head it worked like:

1) When loading the page, React renders the HTML5 Canvas(es) once 2) The HTML5 canvas re-renders itself using requestAnimationFrame with whatever values are found in curValues at some given FPS instead of letting React re-render it.

My understanding was that values are fetched fairly fast (every 10ms iirc), but we don't have to update the plot nearly as fast since we're restricted by the refresh rate of the monitor (e.g. 60Hz for me, or 16.6ms). requestAnimationFrame syncs up with the monitor's refresh rate so it made sense to let that handle it.

Honestly I don't know how much this makes sense from an actual webdev standpoint but it made sense in my head. This was the first time I worked with React, and also combining it with HTML5 Canvases so I'm definitely open to learning more about the process.

I think I might need a better understanding as to why adding dependencies is better here as opposed to the previous approach before I can comment on which approach I think is more desirable.

[joshhead]

What you're describing sounds accurate to me and I think it's a fine approach, especially for curValues. It still works that way with my proposed change.

I found an article that isn't too long and I think has good advice about never ignoring react-hooks/exhaustive-deps:

There are a couple reasons this is worth the effort:

Your probably actually do want to fire the effect (or whatever hook you’re dealing with) when the variable changes. It might not seem like it at first, but there are probably some edge cases for which your effect logic will fail if you don’t include the variable in the dependency array. Once you add the // eslint-disable-next-line, you have now given up the opportunity to be warned about future dependencies. If you add a new dependency to the effect, you won’t be warned about forgetting to include it in the dependency array. This could be a big deal and break your app.

https://typeofnan.dev/you-probably-shouldnt-ignore-react-hooks-exhaustive-deps-warnings/

That is not to say you shouldn't have an effect that manages rendering to a canvas in its own requestAnimationFrame loop.

If you know that a variable is stable (it won't change between renders), you can just safely keep it in the dependency array knowing that it will never trigger an effect.

https://dev.to/wkrueger/never-ignore-the-exhaustive-deps-rule-2ap8

As long as EmitValue, curThresholds, curValues, index, and webUIDataRef don't change (by reference equality), the effect will still only run once. To my understanding, they don't. Maybe it would be better practice to pass only the whole ref to the effect instead of the the arrays 🤔

[teejusb]

All of that makes sense! I wasn't quittteeee sure if it was by reference equality or by the contents (but reference does make sense)! All in all the changes look good to me and I'll merge after I get a chance to manually test it myself with my pad. Thanks for this!

[teejusb]

I tested it and it seemed to work great!

[joshhead]

Awesome, thanks :)