introduce React Hooks for handling state in WebUI

[proddy]

One thing I wanted to try out is replacing the class-based functions (HOCs) that handle local component states with React Hooks. Essentially using functional components to handle the setting of, listening to and updating state. It would make the code slightly easier to read, reduce the boilerplate size (e.g. the onComponent* calls can be replaced with useEffect()) and the state won't be passed around the map.

[proddy]

First implementation is a hook for loading and saving settings data instead of extending RestControllerProps and using the HOC. The hook loads the data on mount and exposes the data, error message (if applicable) and save/load functions and a saving flag (boolean). This saving flag can be used so the form inputs may be disabled instead of the loading indicator which we currently use which causes the annoying "Loading..." flicker on form submits.

Doing this work in the ft_reacthooks branch

[proddy]

at the same time I'm upgrading to MUI5 (material-ui)

[proddy]

and while I'm at it, I'll use JSON msgpack as default to compress the payloads.

[proddy]

Most of the work is done, a lot in the backend and a very cosmetic changes. This will be the basis for v3.4. The UI looks a little different now. It's in https://github.com/emsesp/EMS-ESP32/tree/ft_reacthooks if anyone wants to test drive it (@bbqkees, @MichaelDvP)

[bbqkees]

Look great I'll try it out this week.

[MichaelDvP]

First test results:
Looks nice and code is better readable (as promised).

• The first commits causes some git issues, it could be fetched but checkout or merge was not possible. Later i see that it was a local path (C:\users\paul) in the fonts path (already removed). Because of this i loaded the zip and merged manual to my build. It's more work, but i can read and understand the code better.
• You've updated most packages and changed nearly every file, is there a reason why you stay with react-router 5.3?
• mqtt path crashes for guest users, mqtt-status is set as Admin-route and redirect is to status, this causes a loop and esp crashes. Remove admin from status and add to settings fixes this.
• Settings is highlighted for guest-users, but redirects to dashboard, no issue, but hiding the settings here with disabled={!authenticatedContext.me.admin} would be better.
• The dashboard uses for the device data the deviceData.id which is send to esp generate_values_web and resend by generate_values_web (pingpong over ip) and also stored local in browser as selectedDevice. I've only use selectedDevice and have removed devcieData.id completely, no issue. Have i missed the reason for this construction? It's not intuitive.
• The device value dialog shows numbers with a lot of digits and calculation errors. Better to use here value={deviceValue.u ? Intl.NumberFormat().format(deviceValue.v) : deviceValue.v}
• After updating with changes of web page it has to be reloaded in browser to show correctly. An automatic reload can be done by inserting after this line window.location.reload(); or window.location.assign("/"); for redirect to dashboard. But i fear the redirect will make people complaining because in this early stage after reboot there is always the no ems devices warning.
• renaming of enum-format is mixed, right in value-info, but wrong in generate_values. I think value is ambiguous, the index is also a value. Better call the options name and index.
• What i wanted to do since long time is a automatic reload of data in dashboard. With the new page this only a few lines (30 sec refresh), great (works, but causes a eslint warning):

const reloadData = () => {
    loadData();
    if (selectedDevice) {
      fetchDeviceData(selectedDevice);
    }
  };

  useEffect(() => {
    const timer = setInterval(() => reloadData(), 30000);
    return () => {
      clearInterval(timer);
    };
    // eslint-disable-next-line
  }, [selectedDevice]);

[proddy]

First test results: Looks nice and code is better readable (as promised).

Thanks Michael. This is great feedback. I've been working on the UI since that last push to the branch so a few things have improved. I'll check in tomorrow when I'm back from travelling

• The first commits causes some git issues, it could be fetched but checkout or merge was not possible. Later i see that it was a local path (C:\users\paul) in the fonts path (already removed). Because of this i loaded the zip and merged manual to my build. It's more work, but i can read and understand the code better.

This was fixed. The dynamic font loading didn't work and increased the size of the package so it's back to hardcoding the fonts in the app and including them in the public folder.

• You've updated most packages and changed nearly every file, is there a reason why you stay with react-router 5.3?

React-Router 6 is a beast. It changes the logic so a lot needed to be adjusted. It's done though and in my latest build. You'll see now that the code is even easier to read which is nice.

• mqtt path crashes for guest users, mqtt-status is set as Admin-route and redirect is to status, this causes a loop and esp crashes. Remove admin from status and add to settings fixes this.

To be honest I hadn't checked the access when using a non-admin user. I checked just now and yes the MQTT form would break so I'll fix that. Thanks for pointing it out.

• Settings is highlighted for guest-users, but redirects to dashboard, no issue, but hiding the settings here with disabled={!authenticatedContext.me.admin} would be better.

Settings should be 'greyed' out - it should be fixed in the latest

• The dashboard uses for the device data the deviceData.id which is send to esp generate_values_web and resend by generate_values_web (pingpong over ip) and also stored local in browser as selectedDevice. I've only use selectedDevice and have removed devcieData.id completely, no issue. Have i missed the reason for this construction? It's not intuitive.

Not sure, can't remember! I will ill look into it and get back to you. I'm all for optimizing for performance where possible.

• The device value dialog shows numbers with a lot of digits and calculation errors. Better to use here value={deviceValue.u ? Intl.NumberFormat().format(deviceValue.v) : deviceValue.v}

Pretty sure that has been fixed too in my latest. If not let me know.

• After updating with changes of web page it has to be reloaded in browser to show correctly. An automatic reload can be done by inserting after this line window.location.reload(); or window.location.assign("/"); for redirect to dashboard. But i fear the redirect will make people complaining because in this early stage after reboot there is always the no ems devices warning.

This has been on my list for a while (#178) and I have a working prototype. I'll see if I can merge it later this week.

• renaming of enum-format is mixed, right in value-info, but wrong in generate_values. I think value is ambiguous, the index is also a value. Better call the options name and index.

Yes that was one of the many fixes I made in the last weeks.

• What i wanted to do since long time is a automatic reload of data in dashboard. With the new page this only a few lines (30 sec refresh), great (works, but causes a eslint warning):

const reloadData = () => {
    loadData();
    if (selectedDevice) {
      fetchDeviceData(selectedDevice);
    }
  };

  useEffect(() => {
    const timer = setInterval(() => reloadData(), 30000);
    return () => {
      clearInterval(timer);
    };
    // eslint-disable-next-line
  }, [selectedDevice]);

This has also been top of mind for a while. The proper way to do this is using WebSockets and keeping a connection over, and have EMS-ESP push out the changes. It's not hard to implement for a simple form but my design is quite complex as there are two feeds. This is worthy of a separate GH issue to spend some time designing the right solution. Personally, I don't like the interval() and forced refresh() - this is how I implemented it back in version 1.9.,

So, a lot of changes in this branch, also in the core C++ code so watch out if you plan to merge with your dev branch. This branch has a new customization service so I refactored some of your Dallas code into that

[proddy]

it's all in https://github.com/proddy/EMS-ESP32/tree/ft_reacthooks if you want to take a first look. When I've finished changing the HA MQTT for Dallas I'll create a PR for the official ft_reacthooks and then probably merge it with dev to get feedback from other power users.

[proddy]

I've merged the latest build. Please @MichaelDvP and @bbqkees take a look at let me know what you think. A lot of changes and of course not fully tested yet.

@MichaelDvP this build should address all your points above, except for the auto-refresh of the deviceData. Also your comment on deviceData.id ping-pong I couldn't reproduce. I may have missed something so do let me know if that irregularity is still there.

[MichaelDvP]

A lot of changes, haven't tested yet. For now only on thing, also in the first build: This should be a GET to match this.

[MichaelDvP]

Tested a bit more..
No issues found except not working scan devices (GET/POST as mentioned).
UI looks nice, good idea to move the help to the menu.

Some remarks:

Dallas refactor:

• removed dallas format option, why? Nested is now only with leading ID (what you doesnt like all the time).
• removed sensorname command in telnet, sad, i use this to set all sensors by a single copy&paste, but not important for other users.
• also not possible anymore to set a sensor that is currently not attached.
• sensornames can not be removed from filesystem, the list will grow with every new sensor.
• broken/missing dallas show 3200.0 °C in web.

others:

• rename of wwdisinfect to wwdisinfecting is right for boiler, but not for thermostat
• ww3wayon sounds odd if combined with on/off setting. Better ww3wayvalve or ww3waycharge
• all services have a enable option (mqtt, AP, syslog, ...), but for telnet you use a disable option, inconsistent.
• the link from system/version information to upload should be in the same window (not target=blank)

[proddy]

thanks for testing. I've done some more work since which I'll upload tonight if i can

scan devices (GET/POST as mentioned)

Fixed a while back, need to re-test

removed sensorname

I didn't see the point since the web is used to configure and telnet only for debugging and critical system commands like initializing a board or connecting to the wifi. Looks like you could do with a new feature to load customization settings

also not possible anymore to set a sensor that is currently not attached

but that's like renaming a file that doesn't exist? I don't see the use case yet...

sensornames can not be removed from filesystem, the list will grow with every new sensor

good point, not critical but worth purging the list at some point, or resetting the customization or loading a previous file

broken/missing dallas show 3200.0 °C in web

haven't seen that. How can I simulate it?

rename of wwdisinfect to wwdisinfecting

can you comment in #211

ww3wayon sounds odd if combined with on/off setting. Better ww3wayvalve or ww3waycharge

also comment in #211 please

all services have a enable option (mqtt, AP, syslog, ...), but for telnet you use a disable option, inconsistent.

good point! I'll change

the link from system/version information to upload should be in the same window (not target=blank)

Fixed a while back, need to re-test

[MichaelDvP]

broken/missing dallas show 3200.0 °C in web

haven't seen that. How can I simulate it?

unplug a sensor and wait 30 sec, the sensorvalue goes to NOT_SET.

My solution is in WebDataService.ccp:

if (Helpers::hasValue(sensor.temperature_c)) {
    obj["t"] = (float)sensor.temperature_c / 10;
}

and in DashboardData.tsx, formatValue:

if (value === undefined) {
      return '-';
}

[proddy]

got it, made the change. called it offline. uploading now...

[proddy]

@MichaelDvP I wasn't sure what you first meant by

The dashboard uses for the device data the deviceData.id which is send to esp generate_values_web and resend by generate_values_web (pingpong over ip) and also stored local in browser as selectedDevice. I've only use selectedDevice and have removed devcieData.id completely, no issue. Have i missed the reason for this construction? It's not intuitive.

So I had a look at your fork. You're correct and I've adapted your change to DashboardData.tsx. But....I also saw a lot of really nice additions you made so I took the liberty of merging them into too. Things like the publish_single(), dallas temperatures in Fahrenheit, io_counter etc. Hope you don't mind!

[MichaelDvP]

The fahrenheit implementation is a mess, as i started is seems easy, but it's getting more and more.. (like checks for relative/absolut values, etc).

I took the liberty of merging them into too

You're wellcome, that's why it is online.

The publish single is optimal for ioBroker, i don't want to miss it. For HA i think it's better to use the json, scheduled publish. I think it works, but never tested with HA. (Without HA schedule time zero disables json publish if publish on change is done by single).

[proddy]

I started to find a nicer way to render the Fahrenheit temperatures and gave up, went back to your way by adding the Fahrenheit parameter to the Helper render functions. If it works, its fine for now. We can always refactor later.

[proddy]

also @MichaelDvP shall I add the analog to the new customization service like it is with the Sensors (you can change the name) or not? Or do you want to do it later?

[proddy]

The publish single is optimal for ioBroker, i don't want to miss it

I've added it, not tested yet, but merged your code. I always assumed setting MQTT formatting "as individual topics' did the same thing?

[MichaelDvP]

shall I add the analog to the new customization service

yes there it belongs now. I've added this a few days ago before you've made the custum.
I planed to move to Customization but also expand to 4 channel (ADC0 and ADC1, channel 0/1, gpio 34, 35, 36 39).
I'm using only one for a pressure sensor (0,5-4,5V), my boiler does not have one and no connection for it.
I set the adc-scale fixed here. Could make sense to make it selectable. (afair there was a bug in arduino-lib, that the channels can not set individual)

[proddy]

ok i'll add it to the customization, just to rename the name and we can add the expansion to the ports later. I saw that adc-scale fix aswell. It was included in the last tasmota release a few weeks ago.

[proddy]

I'll create a new class called analog.cpp like I did with dallas

[proddy]

it's part of the v3.4 branch. I'll remove ft_reacthooks at some point.