rjwats / esp8266-react

A framework for ESP8266 & ESP32 microcontrollers with a React UI
GNU Lesser General Public License v3.0
478 stars 147 forks source link

Question: using React Hooks for managing state? #251

Closed proddy closed 2 years ago

proddy commented 3 years ago

One thing I haven't had the guts to do is to look at replacing the class-based functions 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 code size (e.g. the onComponent* calls can be replaced with useEffect()) and the state won't be passed around the map. Functionally there is zero gain. And oh I think Redux is slightly overkill right now.

I don't have a lot of experience in this area so unsure where to begin, or whether it's going to be worthwhile other than a nice learning experience.

thoughts anyone?

rjwats commented 3 years ago

Yes. In my more recent React projects, at home and professionally, have all but abandoned class based components, hooks are great. A well designed hook can reduce boilerplate hugely.

I had it in mind to revamp the UI design a bit, change out a few of the more dated libraries (mainly the validator) and ditch the class components (HOCs) to use hooks instead.

It's been a while since I gave this project much attention but ive been thinking of this change in particular because coming back to this project from my others does feel like a step backwards (or shall we say it has a dated feel)

On Thu, 19 Aug 2021, 20:23 Proddy, @.***> wrote:

One thing I haven't had the guts to do is to look at replacing the class-based functions 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 code size (e.g. the onComponent* calls can be replaced with useEffect()) and the state won't be passed around the map. Functionally there is zero gain. And oh I think Redux is slightly overkill right now.

I don't have a lot of experience in this area so unsure where to begin, or whether it's going to be worthwhile other than a nice learning experience.

thoughts anyone?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VGBNYABMPNLMAEOJJTT5VK4BANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

rjwats commented 3 years ago

I had some particular functional changes to the UI in mind which could do with another set eyes

On Thu, 19 Aug 2021, 20:35 Rick Watson, @.***> wrote:

Yes. In my more recent React projects, at home and professionally, have all but abandoned class based components, hooks are great. A well designed hook can reduce boilerplate hugely.

I had it in mind to revamp the UI design a bit, change out a few of the more dated libraries (mainly the validator) and ditch the class components (HOCs) to use hooks instead.

It's been a while since I gave this project much attention but ive been thinking of this change in particular because coming back to this project from my others does feel like a step backwards (or shall we say it has a dated feel)

On Thu, 19 Aug 2021, 20:23 Proddy, @.***> wrote:

One thing I haven't had the guts to do is to look at replacing the class-based functions 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 code size (e.g. the onComponent* calls can be replaced with useEffect()) and the state won't be passed around the map. Functionally there is zero gain. And oh I think Redux is slightly overkill right now.

I don't have a lot of experience in this area so unsure where to begin, or whether it's going to be worthwhile other than a nice learning experience.

thoughts anyone?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VGBNYABMPNLMAEOJJTT5VK4BANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

proddy commented 3 years ago

Ok, would love to help using hooks & functional components instead of HOCs. Reducing the boilerplate is a good idea. And a few of the packages need updating too.

rjwats commented 3 years ago

Have you done any work on this yet, or happy for me to have a look this eve? I think replacing the HOCs with hooks should be easy enough to do in an evening...

proddy commented 3 years ago

No, haven't touched any code. Wasn't sure where to start (it's new to me) and what problems I'd get with mixing component & functional classes with props. Happy to help and test though

rjwats commented 3 years ago

Cool, i'll send a PR over this eve for you to review/comment on.

On Sat, Aug 21, 2021 at 2:46 PM Proddy @.***> wrote:

No, haven't touched any code. Wasn't sure where to start (it's new to me) and what problems I'd get with mixing component & functional classes with props. Happy to help and test though

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-903118730, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VEKACEMQWDYBJUO5ITT56U4JANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

rjwats commented 3 years ago

@proddy Here's an early work-in-progress PR with an example of using a hook for loading/saving 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. I never really liked the flickering of the forms when you submit them, this is a result of the "loading" boolean being used for both saving and loading with the current approach.

I think dropping the controller components (WiFiStatusController/WiFiSettingsController etc...) probably worth considering, they don't do much and add to the boilerplate which isn't really required when you have a hook for a context for dealing with the loading of the data. The intention of them was to separate data loading/saving from the forms but the hook does that now. It does mean some logic in the components to choose when to render the loading indicators but this also affords better flexibility and using skeletons for loading indicators is a nicer option than the indeterminate LinearProgress components we currently use everywhere.

For your input, I have updated FeaturesWrapper to a functional component and used the new hook (it previously did it's own loading) and also LightStateRestController has been updated as an example of a component which loads and saves.

Thoughts on this so far?

proddy commented 3 years ago

at first glance, I think I'm following the logic! I like the clean separation and having the FeaturesWrapper not as context. I'll dig into it more tonight and try out a few things. Other than removing the controller components are there any other core changes needed?

rjwats commented 3 years ago

I'll need to implement a websocket equalivant... and revisit all of the form components to make proper use of the disabled flag.

The users table + form (modal) is more complicated and will require some work to turn into a functional component.... not too hard though. Finally the WiFi selector is a somewhat more complicated but should be easy to rework into a functional component.

On Mon, 23 Aug 2021, 12:56 Proddy, @.***> wrote:

at first glance, I think I'm following the logic! I like the clean separation and having the FeaturesWrapper not as context. I'll dig into it more tonight and try out a few things. Other than removing the controller components are there any other core changes needed?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-903697332, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VHGY4KE6A5VPBK6A2TT6IZOTANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

proddy commented 3 years ago

hey, I've been busy with work, illness etc but do plan to take a look and integrate these awesome additions into my project soon

rjwats commented 3 years ago

Nice, I still intend to do the same for the framework ui, it's well overdue. Busy weekends recently have left me with little time sadly :)

On Sun, 12 Sep 2021, 17:09 Proddy, @.***> wrote:

hey, I've been busy with work, illness etc but do plan to take a look and integrate these awesome additions into my project soon

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-917664409, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VEK5R7P63B6B5VEOVDUBTGEPANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 3 years ago

ok, so I started applying the hooks to my project and converting some of my controller/form pages. So far so good although I'm seeing a few quirks with the Form Validator and I remember you mentioned you wanted to swap out some of the older libraries for something new. What did you have in mind for form field validation? (especially checking hostnames, IPv4/6 IP addresses etc). Then I'll give that a shot

rjwats commented 3 years ago

Hey,

Sorry, I drafted a reply a while ago and got distracted.

I'm not really sure what to recommend, we have been using "async-validator https://github.com/yiminghe/async-validator" at work. This library requires a slightly different approach where you author your validators, run them against the form state, then do something with the result (such as rendering errors under inputs). It's not designed to integrate with a component library, so that's up to you, but I quite like being able to separate my forms from my validators, it's possibly more useful for a larger project where forms are repeated (or parts of forms) throughout the project. It's also sometimes handy to validate some state without needing it to be form-backed.

Other than that, I know react-hook-form https://react-hook-form.com/ is trendy at the moment, and it's not too large and I believe it integrates with various component libraries right off the bat... having not used it yet I can't really comment with any authority.

MUI 5 is out, so I'm stuck with updating some work projects. Likewise the framework needs a similar refresh, it feels like the technical-debt in the framework is adding up to something quite substantial now :D

Rick

On Fri, Sep 24, 2021 at 8:38 PM Proddy @.***> wrote:

ok, so I started applying the hooks to my project and converting some of my controller/form pages. So far so good although I'm seeing a few quirks with the Form Validator and I remember you mentioned you wanted to swap out some of the older libraries for something new. What did you have in mind for form field validation? (especially checking hostnames, IPv4/6 IP addresses etc). Then I'll give that a shot

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-926875266, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VDILTPKMHZNPDU7G6TUDTHRRANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 3 years ago

I started to upgrade to mui 5 but got caught up in a jumble of dependency poo so gave up. I'm still trying to learn hooks and slowly converting the components/*.tsx which is not easy. I'm stuck at MenuAppBar.tsx!

The form validators is a nice-to-have. I think getting away from class components/HOCs and adding the i18n support are priorities for me at the moment.

Would be great if we could find a Front-end React/TS developer that can help with tackling some of the tech-debt and migrations.

proddy commented 3 years ago

I'm back on the Hooks again and had a bash at converting some of common classes over. What was your idea behind the HOCs (like export default withAuthenticatedContext(withTheme(NTPStatusForm)))? Create custom hooks for these?

rjwats commented 3 years ago

Humm, not quite.

Contexts can be automatically injected using the "useContext" hook provided by React.

should be as easy as:

const authenticatedContext = useContext(AuthenticatedContext);

Rick

On Sat, Oct 2, 2021 at 8:13 PM Proddy @.***> wrote:

I'm back on the Hooks again and had a bash at converting some of common classes over. What was your idea behind the HOCs (like export default withAuthenticatedContext(withTheme(NTPStatusForm)))? Create custom hooks for these?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-932806520, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VASOSXSTU4DXTKSMVTUE5KWFANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 3 years ago

right, think I'm going to throw in the towel on this one. It's way too complex for me. I managed to migrate my projects controller/forms over but really want the core to not use HOCs and couldn't get past MenuAppBar.tsx even after taking a few days off work to teach my self react hooks.

rjwats commented 3 years ago

Class components aren't going anywhere any time soon so what you have isn't suddenly going to become unworkable or unsupported :)

I've become a big advocate of FC's now and all of my recent projects (including @ work) have been exclusively FC based because it seriously reduces the amount of boilerplate you have to write :)

I still have it in mind to revisit the project and update it (the UI) to use functional components and replace the validator library with yup... It's probably only a weekend's work for the framework (which is much simpler than your project o/c). I'm currently in the middle of another personal project. Hopefully over the half-term break I'll have some time for this :)

On Sat, Oct 23, 2021 at 10:29 AM Proddy @.***> wrote:

right, think I'm going to throw in the towel on this one. It's way too complex for me. I managed to migrate my projects controller/forms over but really want the core to not use HOCs and couldn't get past MenuAppBar.tsx even after taking a few days off work to teach my self react hooks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-950123672, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VDM3MIJAW2YFRHR3NDUIJ565ANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 3 years ago

can't wait and happy to help and test. My main driver for the HOC conversion was to enable the implementation of i18n which uses contexts and I couldn't bootstrap it into the render() functions. So started top-down with the AppMenu. Migrating the form/controllers was easy enough

tichaonax commented 3 years ago

What is the status of this? Is it ready to merge to master. What is the impact/changes on existing projects. Is it good to go to use the branch without necessarily merging into master

rjwats commented 3 years ago

Hi Tichaona,

Very much a work in progress still. Enough has changed in the React world to warrant a significant re-write of the UI:

I've started on this now and have made decent progress locally, will be pushing a branch shortly if you're interested in getting some early visibility.

I'm looking to deprecate the HoC approach, but will keep the HoC's in the UI for a transitional period.

Rick

On Mon, Nov 1, 2021 at 2:21 PM Tichaona Hwandaza @.***> wrote:

What is the status of this? Is it ready to merge to master. What is the impact/changes on existing projects. Is it good to go to use the branch without necessarily merging into master

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-956277045, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VFJ55XSW3MYE6OIJJ3UJ2PALANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 3 years ago

can't wait! I've put in many of my free days trying to do the port myself with variable success so looking forward to learning from Rick on how to do it properly!

rjwats commented 3 years ago

Working on this in this branch if you want some early visibility:

https://github.com/rjwats/esp8266-react/tree/%23251-NewUI

Effecively re-building the UI using mui 5, hooks and a new validator but recreating it like-for-like style/functionality wise.

Have done the sign-in page, app bar (layout) and auth/router bits, moving on to the forms now which I imagine to be easier that what I've done so far.

proddy commented 3 years ago

looks really nice so far, learning loads. I'm taking a few days off next week to work on my port. I'll provide feedback but the stuff you're doing is well beyond my skill grade

proddy commented 2 years ago

I started porting my stuff over to see how I get and really enjoying the new async-validator and also how you're using axios and promises to be truly asynchronous. Can't wait for those final bits so I can take it for a real spin 'round the block

rjwats commented 2 years ago

It's a decent start I think. I'll have some time this weekend to progress it. Weeks are crazy busy :)

On Thu, 11 Nov 2021, 19:33 Proddy, @.***> wrote:

I started porting my stuff over to see how I get and really enjoying the new async-validator and also how you're using axios and promises to be truly asynchronous. Can't wait for those final bits so I can take it for a real spin 'round the block

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-966569929, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VAEGFJAEGP3UQBEE63ULQLCDANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 2 years ago

how can I encourage you? :-) send over some wine & cheese?

rjwats commented 2 years ago

Haha! No encouragement required! I want to get this upgraded before the Christmas tree goes up :) I need to figure out the hook based websocket suff then it's plain sailing

On Sat, 13 Nov 2021, 16:27 Proddy, @.***> wrote:

how can I encourage you? :-) send over some wine & cheese?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-968095044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VFSZBGWC3KEQCSMR2LUL2GXBANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rjwats commented 2 years ago

Websocket hook stuff done.. firmware upload system page updated... last few forms left now

On Sat, 13 Nov 2021, 19:57 Rick Watson, @.***> wrote:

Haha! No encouragement required! I want to get this upgraded before the Christmas tree goes up :) I need to figure out the hook based websocket suff then it's plain sailing

On Sat, 13 Nov 2021, 16:27 Proddy, @.***> wrote:

how can I encourage you? :-) send over some wine & cheese?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-968095044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VFSZBGWC3KEQCSMR2LUL2GXBANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

rjwats commented 2 years ago

This will need a bit of testing, and i have a few tidyups in mind.

It's just the security section left to convert now which should be able to get done this eve.

On Tue, Nov 16, 2021 at 10:32 PM Rick Watson @.***> wrote:

Websocket hook stuff done.. firmware upload system page updated... last few forms left now

On Sat, 13 Nov 2021, 19:57 Rick Watson, @.***> wrote:

Haha! No encouragement required! I want to get this upgraded before the Christmas tree goes up :) I need to figure out the hook based websocket suff then it's plain sailing

On Sat, 13 Nov 2021, 16:27 Proddy, @.***> wrote:

how can I encourage you? :-) send over some wine & cheese?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-968095044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKE4VFSZBGWC3KEQCSMR2LUL2GXBANCNFSM5CO3VAHA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

proddy commented 2 years ago

Very nice work! I’ve been following all the updates (and learning loads) and manually merging into my project. I’m ready to test and provide feedback…

On Sun, 21 Nov 2021 at 16:25, rjwats @.***> wrote:

This will need a bit of testing, and i have a few tidyups in mind.

It's just the security section left to convert now which should be able to get done this eve.

On Tue, Nov 16, 2021 at 10:32 PM Rick Watson @.***> wrote:

Websocket hook stuff done.. firmware upload system page updated... last few forms left now

On Sat, 13 Nov 2021, 19:57 Rick Watson, @.***> wrote:

Haha! No encouragement required! I want to get this upgraded before the Christmas tree goes up :) I need to figure out the hook based websocket suff then it's plain sailing

On Sat, 13 Nov 2021, 16:27 Proddy, @.***> wrote:

how can I encourage you? :-) send over some wine & cheese?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/rjwats/esp8266-react/issues/251#issuecomment-968095044 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAKE4VFSZBGWC3KEQCSMR2LUL2GXBANCNFSM5CO3VAHA

. Triage notifications on the go with GitHub Mobile for iOS < https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675

or Android < https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rjwats/esp8266-react/issues/251#issuecomment-974837135, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMO6DEALAQJQGWPODKWIDUNEFN5ANCNFSM5CO3VAHA .

proddy commented 2 years ago

I upgraded my project to use the latest react hooks #251 branch. It took a few evenings but really worth it! Really nicely done Rick, thanks.

Most of my changes are cosmetic but I did also make a few alterations to some of the features in line with my customized back-end code. All these changes including questions and suggestions are here.

rjwats commented 2 years ago

I think I've made some similar cosmetic changes and added a new MessageBox component for some de-duplication. Also moved some functions around to remove some redundant checks - It's getting there.

Didn't spot this - will look at this and get back to you.

Not sure what you mean by this, can you elaborate? Is something missing in the UI here?

Fontsource is bringing both woff and woff2 - I think we may have to revert to manually including the fonts again because it doesn't look like you can choose only woff2, no biggie, but that should get the size back to about 160k:

https://github.com/fontsource/fontsource/blob/main/packages/roboto/latin-500.css

When I started this v6 was in beta, worth upgrading

Indeed, there are lots of dependencies which won't affect the built app but would however cause issues if you were using those libraries on a server side application for instance. It's a tricky one really, because you either have to either:

In the past, upgrading CRA has usually removed all of the serious issues for me, however CRA hasn't been updated in an age and there are quite a few issues at the moment!

Personally I'm wary of turning off dependency scanning and feel like it's too much effort to keep a list of suppressions up to date :) Not least because some issues have historically affected the dev server making your dev PC vulnerable (the DNS rebinding attack which affected the webpack-dev-server a while back is the one which jumps to mind)

I like the idea of a syslog feature!

proddy commented 2 years ago

The MessageBox component is a nice addition, added that!

Re the AccessPoint missing params, that was my bad. I remember removing them a while back.

I'm ok with living with the GH vulnerabilities too - it's not worth the effort to keep it updated and you're right, CRA is getting a bit stale.

I just noticed a font flash (FOUT) in the Tabs when rendering. It could be only on my side - I'll take a look

rjwats commented 2 years ago

I'm going to upgrade react router, then I think this is "done".

I coudn't re-create the NaN validation issue, is this on one of the framework pages? Have you got steps to re-produce?

proddy commented 2 years ago

"once you pop you can't stop", I keep on adding things too!

The NaN error, go to any settings form which has ValidateTextField with type number and empty it, e.g. the Port in the OTA screen:

image

rjwats commented 2 years ago

I getcha, thanks for testing and for clarifying. I probably read your comment too quickly and assumed it was somthing you were seeing in the UI rather than in the console.

Slightly awkward one: event.target.valueAsNumber can return NaN if the input of type "number" does not contain a valid number (of which an empty string certainly isn't).

I've pushed a fix, though if we have optional number fields it may be sub-optimal still because NaN serialises to null which was news to me and may not be desirable:

image

I tried to find something elegant, such as changing the implementation of extractEventValue so it works seamlessley, but because the value has to be turned into a string at some point there's no getting around converting a NaN or undefined into an empty string when setting the input value, otherwise you risk flipping the component from controlled to uncontrolled, though perhaps I've missed somthing blatant :)

rjwats commented 2 years ago

Well that's been an intresting 2.5 hours of exploring migrating to react router 6.

It's more involved than it looks. Lots of simple naming changes and some minor adjustments around how the routing works (no more "exact" prop, change in how to do redirects, etc). The most tricky migration is around the composition of custom routes: https://gist.github.com/mjackson/d54b40a094277b7afdd6b81f51a0393f

I have something working in a local branch, but I want to give it more testing. I'm thinking perhaps I merge the PR as-is as and get this in a follow up.

proddy commented 2 years ago

ok. To be honest I did try the conversion myself too thinking it would be a simple search & replace, but gave up when I hit the custom route nightmare!

proddy commented 2 years ago

I just noticed a font flash (FOUT) in the Tabs when rendering. It could be only on my side - I'll take a look

Those FOUT (Flash of Unstyled Text) was bothering me so I went back to just including all the fonts in package.json. The flashes happen on all the Tabs and also the SectionContext title. It's subtle but noticeable.

rjwats commented 2 years ago

humm right, i have noticed that too, perhaps the old way was the best (which was including it as a static resource in /public)

rjwats commented 2 years ago

Looks like I managed to neglect the unicode-range in the CSS which seems to solve the problem for me.

proddy commented 2 years ago

ah good!