Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: using React Hooks for managing state? #251

Closed
proddy opened this issue Aug 19, 2021 · 43 comments · Fixed by #262
Closed

Question: using React Hooks for managing state? #251

proddy opened this issue Aug 19, 2021 · 43 comments · Fixed by #262

Comments

@proddy
Copy link

proddy commented Aug 19, 2021

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
Copy link
Owner

rjwats commented Aug 19, 2021 via email

@rjwats
Copy link
Owner

rjwats commented Aug 19, 2021 via email

@proddy
Copy link
Author

proddy commented Aug 19, 2021

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
Copy link
Owner

rjwats commented Aug 21, 2021 via email

@proddy
Copy link
Author

proddy commented Aug 21, 2021

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
Copy link
Owner

rjwats commented Aug 21, 2021 via email

@rjwats
Copy link
Owner

rjwats commented Aug 22, 2021

@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
Copy link
Author

proddy commented Aug 23, 2021

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
Copy link
Owner

rjwats commented Aug 23, 2021 via email

@proddy
Copy link
Author

proddy commented Sep 12, 2021

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
Copy link
Owner

rjwats commented Sep 12, 2021 via email

@proddy
Copy link
Author

proddy commented Sep 24, 2021

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
Copy link
Owner

rjwats commented Sep 30, 2021 via email

@proddy
Copy link
Author

proddy commented Sep 30, 2021

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
Copy link
Author

proddy commented Oct 2, 2021

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
Copy link
Owner

rjwats commented Oct 3, 2021 via email

@proddy
Copy link
Author

proddy commented Oct 23, 2021

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
Copy link
Owner

rjwats commented Oct 23, 2021 via email

@proddy
Copy link
Author

proddy commented Oct 25, 2021

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
Copy link

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
Copy link
Owner

rjwats commented Nov 2, 2021 via email

@proddy
Copy link
Author

proddy commented Nov 2, 2021

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
Copy link
Owner

rjwats commented Nov 5, 2021

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
Copy link
Author

proddy commented Nov 6, 2021

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
Copy link
Author

proddy commented Nov 11, 2021

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
Copy link
Owner

rjwats commented Nov 11, 2021 via email

@proddy
Copy link
Author

proddy commented Nov 13, 2021

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

@rjwats
Copy link
Owner

rjwats commented Nov 13, 2021 via email

@rjwats
Copy link
Owner

rjwats commented Nov 16, 2021 via email

@rjwats
Copy link
Owner

rjwats commented Nov 21, 2021 via email

@proddy
Copy link
Author

proddy commented Nov 21, 2021 via email

@proddy
Copy link
Author

proddy commented Nov 29, 2021

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 rjwats linked a pull request Nov 29, 2021 that will close this issue
@rjwats
Copy link
Owner

rjwats commented Nov 30, 2021

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.

  • Question: TextField validating numbers when empty giving warning "The specified value "NaN" cannot be parsed, or is out of range."

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

  • Question: AccessPoint - need to add referred Channel, Hide SSID options & Max Clients to backend framework library

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

  • Question: build is bigger than pre-hooks. Is that expected, even with out all the fonts?

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

  • Question: why using react-router-dom v5.3 instead of v6?

When I started this v6 was in beta, worth upgrading

  • Question: GitHub complains about security vulnerabilities from create-react-app.

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:

  • put up with the noise
  • maintain a list of suppressions for issues you are happy don't affect you

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
Copy link
Author

proddy commented Nov 30, 2021

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

@proddy proddy closed this as completed Nov 30, 2021
@proddy proddy reopened this Nov 30, 2021
@rjwats
Copy link
Owner

rjwats commented Nov 30, 2021

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
Copy link
Author

proddy commented Nov 30, 2021

"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
Copy link
Owner

rjwats commented Nov 30, 2021

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
Copy link
Owner

rjwats commented Nov 30, 2021

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
Copy link
Author

proddy commented Dec 1, 2021

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!

rjwats added a commit that referenced this issue Dec 1, 2021
@proddy
Copy link
Author

proddy commented Dec 2, 2021

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
Copy link
Owner

rjwats commented Dec 2, 2021

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
Copy link
Owner

rjwats commented Dec 2, 2021

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

@proddy
Copy link
Author

proddy commented Dec 2, 2021

ah good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants