Closed ethanjli closed 3 years ago
@rohanpurohit This PR will almost certainly produce merge conflicts with your other open frontend PRs, we'll need to coordinate on how to merge things - i.e. for PR of yours which we merge into develop first, I'll need to make sure I understand every changed line of code, as I'll probably need to manually "replay" all of your changes on top of my changes. And for any PR of yours which we merge into develop after this PR, you'll need to understand what I changed and what assumptions you may have made which might be violated by the changes I've made in this PR.
@rohanpurohit Since this PR touches a bunch of files, it'd be great if you could test all aspects of the frontend to help look for any user-facing regressions introduced by my refactoring.
@rohanpurohit This PR will almost certainly produce merge conflicts with your other open frontend PRs, we'll need to coordinate on how to merge things - i.e. for PR of yours which we merge into develop first, I'll need to make sure I understand every changed line of code, as I'll probably need to manually "replay" all of your changes on top of my changes. And for any PR of yours which we merge into develop after this PR, you'll need to understand what I changed and what assumptions you may have made which might be violated by the changes I've made in this PR.
yeah sounds good, I did not find time yesterday to work on my PR'S due to emergency reasons, I'm working on them now and I think both approaches are sound, if you are done with the changes on this PR, then I can go through them to try and understand that and make changes accordingly once this PR gets merged.
@rohanpurohit Since this PR touches a bunch of files, it'd be great if you could test all aspects of the frontend to help look for any user-facing regressions introduced by my refactoring.
Yep, I'll definitely do that.
so in my light to moderate testing, everything seems fine on alarm modals, dashboard, and other related features. Although mute button seems to not work anymore, triggered by stopping the backend.
Great, thanks for catching that! I had forgotten to test alarm muting functionality. I'm able to reproduce the regression. I think it's fine if we leave this up to a future PR in one of the upcoming milestones, because there's some nuance we'll have to figure out there, and it will be easier to do so after #339 gets merged:
I will merge develop back into this PR and then merge this PR into develop as prototype-quality code which has been moderately tested, with known issues to be fixed in future PRs but no regressions identified besides alarm muting functionality. For records-keeping:
This PR fixes #312 by making the frontend initialize all backend-derived messages in the store to
null
on startup, instead of initializing with numbers or default values. Because implementing this required changing the types produced by selectors for those messages, as well as changing the reducers forParametersRequest
,AlarmLimitsRequest
, and their standby versions (mainly because those reducers previously had inconsistent implementations and were difficult for me to understand), it led to a large refactor of the store-related code in the frontend (including improvements in organization, naming), with the following main changes:@controller/PARAMETER_COMMITTED
action) and updateCommittedState (which could generate a infinite number of possible actions with the template@controller/{string}_COMMITTED
), except one could theoretically also use updateCommittedState in place of updateCommittedParameter, and action types were unnecessarily stringly typed. Now we have exactly two actions: commitRequest and commitStandbyRequest, and the message type is specified as aMessageType
rather than a string, and the action generators work with Typescript to ensure that the data provided with the action matches the type constraints for the provided message type.alarmLimits
,alarmLimitsRequest
, andalarmLimitsRequestStandby
), now they're broken up into fragments and organized in a more logical way (e.g.alarmLimits.current
,alarmLimits.request
, andalarmLimits.standby
) in a pattern established by a reorganization I did of the event log-related parts of the store in #313, which movednextLogEvents
,expectedLogEvent
, andactiveLogEvents
intoeventLog
. This reorganization allows us to make generic, composable selectors instead of copying the same logic copied over and over. Previously, updating the code would've required way too much copy-pasting, and updating the code would've been error-prone (forgetting to update something in one place). Now the copy-pasting is restricted to copying a function call and just changing its arguments.BACKEND_HEARTBEAT
&BACKEND_CONNECTION_LOST
store logic from controller to app. This is in preparation for moving all connection-related logic into its own slice of the store.Additionally, now the backend sends the following messages to the frontend: AlarmMuteRequest, BatteryPower, ScreenStatus, RotaryEncoder, SystemSettingRequest, and FrontendDisplaySetting. However, the backend doesn't simulate these messages or load them from the filesystem during initialization; instead, the backend simulator merely initializes some default values; the frontend does indeed receive them from the backend.