Webpack support and minor bug fixes.

[KeiranHines]

Description

Features added

• Added usage of localStorage to store the configuration the user had when last using the UI.

  • Open "Home" or "Stats" tabs will remain open.
  • Last theme will persist as the active theme.

• Added support for Webpack. This enabled code to written cleaner as IDE support such as linting will now work.

  • Modified the webserver.h file to be able to serve gzip'd bundles
  • Moved the build output to a /dist directory under site
  • Moved responsibility of building the frontend bundle to npm/yarn
  • Updated bake_site to leverage the new build pipeline.
  • Added ability to build an offline version of the site bundle. This allows the frontend code to be compiled without use of a CDN. This bundle is significantly larger and not recommended unless your devices will not have an active internet connection.

• Added support for eslint as a method of standardizing frontend styling. Usage of this configuration is optional.

• Added a .clang-format file to allow autoformatting relatively close to the existing standards. Usage of clang-format is optional.

Known limitations:

• The site bundle size has increased from 56K to 81K. This is due to an issue with loading @mui from a CDN in which the theme does not load. To address this migration from @mui/styles should be done https://mui.com/system/styles/basics/. If this migration is completed. On the CDN loading was fixed the bundle size is estimated to be around 34K
• Some minor styling inconsistencies due to some library updates and some classes being applied correctly where they weren't previously. e.g.
  • The Drawer is now slightly thinner than previously.
  • The avatar background color is slightly darker.

Contributing requirements

• I read the contribution guidelines in CONTRIBUTING.md.
• I understand the BlinkenPerBit metric, and maximized it in this PR.
• I selected main as the target branch.
• All code herein is subjected to the license terms in COPYING.txt.

[rbergen]

@KeiranHines Thanks for submitting this.

Do I understand correctly that the main additional feature towards the end user is the persistence of tabs and theme across uses of the UI, and the other changes are more of a preparation for further development?

While having a first look at the changes, I noticed that bake_site.py now depends on the presence of a UNIX-like shell. For the tools used in the main pio build flow, of which bake_site.py is one, we really want to retain functionality on non-UN*X systems (read: Windows) as well. That being a premise, I think the changes to bake_site.py need to be made in such a way that they are OS agnostic.

Furthermore, we want to prevent (additional) dependencies where possible as well, as new dependencies increase the threshold for engaging with the codebase. The logic in the current bake_site.py was specifically written to prevent a dependency on a tool like npm, which you now (re)introduce. Do you think it would be reasonably possible to move forward with the UI development without doing this?

[KeiranHines]

@rbergen you are correct on the user facing features and development improvements. As for your comments. The changes are windows and nix compatible. I have compiled this on windows 11 with PowerShell.

While I agree in principle additional dependencies shouldn't be added lightly. For front end development I feel this change is required, and most developers would expect to use npm for front-end work. I'll admit it took me a bit to work out how everything went together without it, especially lacking any imports between files.

While I feel I could move forward with front-end development without the development changes, I feel it would be a disadvantage in development, especially managing imports between files and conflicts in the current system. Not to mention the lack of good development tooling without the changes.

In my opinion this change would lower the barrier of entry of more front-end developers to engage with the code base.

[rbergen]

Okay, I think we may just have reached the point where using npm (or similar tooling) is inevitable. That said, we will have to:

• use it in a way that allows builds on Windows
• clearly document the additional prerequisite in the project documentation (i.e. the main README, or a document that the README clearly refers to).

[KeiranHines]

Noted. I will update the readme and bake site script to inform developers what to install and provide helpful error messaging if not installed.

On the subject of dependencies. Currently a yarn.lock file is used for version management, however installing yarn requires installing node/npm. I could migrate this to an npm package lock instead reducing the dependencies by one. Assuming the community was happy with using npm.

[robertlipe]

I don't speak react, so I have only the one concern in the C++ side.

FWIW, my primary development environment is MacOS these days and npm is already present in my homebrew installation, so that's probably not a problem the Mac users. The kind of MacOS user cross-compiling code like NightDriverLED -probably- already has Homebrew (or equiv) loaded and 'brew install npm' would be all that'd be needed if I understand the objection. I just don't perceive it as a speedbump.

Thanx for taking this on!

[robertlipe]

The presence or absence of an encoding header depends on its LENGTH? That's odd.

If file.encoding is a plain ole C string, that's just guaranteed to not be null (and if it is, this is about to crash anyway) should it just be testing that the string isn't empty? i.e. if (file.encoding[0]) { ...

If there's more to it than that, please explain (in code, if appropriate, where that magic number "25" comes from.

[rbergen]

In principle I totally agree with defensive coding in general, and when handling C strings in particular. In line with what @robertlipe said, it is not necessary to be too defensive though. In this case, the entire EmbeddedWebFile struct is private to the class this function is a member of. That means we can make some assumptions on the proper initialization of the instances that are created. I therefore agree that a simple check if the first character isnt '\0' is enough.

[KeiranHines]

Updated to address this issue.

[robertlipe]

Yay! fewer hardcoded pathnames is a Good Thing.

[rbergen]

Noted. I will update the readme and bake site script to inform developers what to install and provide helpful error messaging if not installed.

Thanks @KeiranHines, I really appreciate that.

On the subject of dependencies. Currently a yarn.lock file is used for version management, however installing yarn requires installing node/npm. I could migrate this to an npm package lock instead reducing the dependencies by one. Assuming the community was happy with using npm.

If we accept npm as a dependency, which we've just decided to do, then we can also use it to install other dependencies like yarn. If the automation embedded in our build tooling were to take care of "installing dependencies if not present" where needed (using npm where appropriate), I would not consider those dependencies additional prerequisites.

Until you opened this PR, "the community" in the contributing sense was @Louis-Riel. He originally introduced the use of npm before removing the "hard dependency" on it by writing the first version of the bake_site.py script you'll now be modifying.

[KeiranHines]

Thanks for the info @rbergen.
My concern with yarn is that is required to be installed globally. While strictly speaking it does get installed via npm, I wouldn't want to be automatically installing system wide packages. Therefore I believe changing from yarn to using npm directly would be of benefit. It sounds like the yarn lock file that was present in the repo was leftover from the removal of npm initially. If that is the case removing the yarn dependency seems like the best way forward.

[rbergen]

@KeiranHines All of that makes sense to me. Please proceed with the approach you think is best, everything considered.

[KeiranHines]

@rbergen @davepl I have updated the branch the changes to the readme to specify installing NPM, and a warning in the bake_site script. I have also included a switch from yarn.lock to package-lock.json to remove the requirement for yarn.

As a side node. I have already worked on the legacy styling method mentioned in the description and have the bundle size down to 68K now. I will put that PR up after this one has been approved and merged.

[rbergen]

I think we're very nearly there! I have one comment that aligns with one @robertlipe already made and one question, see below.

[rbergen]

In principle I totally agree with defensive coding in general, and when handling C strings in particular. In line with what @robertlipe said, it is not necessary to be too defensive though. In this case, the entire EmbeddedWebFile struct is private to the class this function is a member of. That means we can make some assumptions on the proper initialization of the instances that are created. I therefore agree that a simple check if the first character isnt '\0' is enough.

[rbergen]

Update: you can scratch the question, as I tested it myself. Builds on my (Windows) machine!

[KeiranHines]

@rbergen @davepl for the failing build. it looks like its simply the JS bundle being too large. I have already addressed this in another branch. https://github.com/KeiranHines/NightDriverStrip/tree/feature/styled. This updated branch drops the bundle size from the 81K it is here down to 15K. Which is clearly better than the ~50K it was before I started making changes. What would your preferred method for merging these changes in be? I didn't initially push them to this branch because I didn't want to merge too much code at once. But in the interest of getting the builds passing I can.

[rbergen]

@KeiranHines You're quick to respond to the failing build. :)

I was just looking at it myself, and I noticed that OTA updates are disabled for the TTGO project, while it's using a partition scheme that caters for them. So, as far as I'm concerned we can do two things:

• Pull in the changes you plan on opening a new PR for anyway, into this one (how much "bigger" will the PR get, I wonder?)
• Update platformio.ini to switch to the config/partitions_custom_noota.csv partition scheme for the ttgo environment. That will double the space available for the firmware.

I'm basically fine with both approaches. In your opinion, is there a specific reason not to pull in your lined-up changes straight away?

[KeiranHines]

@rbergen the current stats are 27 files changed, +710, -887. It's mostly changing className={...} to sx={...} for changing the new styling APIs.
The main reason I kept it on a new branch, was I wasn't sure how long it would take for me to complete. There is still one minor change (A warning for a variable name case preference) that I want to address before pulling it in straight away. I should be able to get to that today though.

As for the TTGo_noota, I think that change should be done either way if OTA isn't being used.

[rbergen]

As I said, I am okay with switching to the noota scheme for TTGO because that project doesn't currently have OTA enabled anyway, but making that switch does block (re)enabling OTA for TTGO at the partition level. If we can prevent that scenario by allowing another day (or two, if needs be) for you to complete the changes you're now working on, then personally I would prefer that.

The change stats you shared don't make me nervous, so I'm happy to add those to this PR when you're ready to do so.

[KeiranHines]

@rbergen PR is updated again. Bundle size is now html: 435 B, jsx: 8755 B, ico: 6434 B, total: 15 KB. no OTA is now enabled on the TTGO as well.

[rbergen]

Thanks! Let's stick to the old partition scheme for TTGO though, if that project now builds with the OTA partition scheme and the new reduced bundle size. That way, people can choose to enable OTA with one small edit in globals.h, without having to go into editing platformio.ini (and figuring out its structure first).

[robertlipe]

If #594 in platform.ini actually changes the partition table for ttgo devices, this breaks their filesystem on upgrade, doesn't it? We've broken userdata and potentially just clobbered their active B partition, rendering it unbootable if that's the case.

Didn't we agree we can't be casually flipping around partition tables without code to preserve user data is bad?

It's crazy hard for me to read the inheritance of these scons files, but this change was made for a reason, so I assume we're changing it.

Rutger, how about all those whitespace changes?

[rbergen]

Didn't we agree we can't be casually flipping around partition tables without code to preserve user data is bad?

We did, and this is precisely why I don't want to make this change unless it is the only way to get TTGO to build with the updated web UI.
As a matter of fact, I've just checked out this PR and TTGO builds on my machine with the "old" partition scheme, i.e. with the ..._noota.csv line removed from [env:ttgo]. So, let's decide conclusively to stick to the OTA partition scheme for TTGO.

Rutger, how about all those whitespace changes?

The changed file count is identical with and without whitespace changes filtered. That means other things have changed as well, and then whitespace can go along - if that helps consistency in the modified code. Looking at the proposed changes, that seems to be the case.

[KeiranHines]

Thanks all. I have reverted the TTGO change. Sorry there was some confusion on my side with that one.
As for the whitespace changes. Most of them are from my eslint setup 'fixing' indentation on changed files so nested elements are properly indented. Let me know if you'd like me to try to avoid this in the future. I can limit it to just lines changed instead of files, however I feel that creates worse consistency overall.

[rbergen]

Thanks all. I have reverted the TTGO change. Sorry there was some confusion on my side with that one.

That's ok, that's what reviews are for. :)

As for the whitespace changes. Most of them are from my eslint setup 'fixing' indentation on changed files so nested elements are properly indented. Let me know if you'd like me to try to avoid this in the future. I can limit it to just lines changed instead of files, however I feel that creates worse consistency overall.

Generally speaking, whitespace isn't a Thing in the context of this project. Indentation should clarify the structure of the code it's applied to, and that's mainly it. That means that:

• Whitespace in existing files should be left alone unless the "clarify structure" rule is violated in the existing code. This applies particularly if the other changes made in a relevant PR are small.
• We don't merge PRs that only make whitespace changes, unless something is "critically broken" from an indentation perspective.
• We're not blocking PRs that make otherwise meaningful changes because they also change whitespace, as long as the "clarify structure" rule is respected.

In this case I had already taken a closer look at the React side of things in your PR, and concluded it is not only actually a decently comprehensive overhaul of that code, but also a step-up to future changes that you've already indicated you want to work on. Furthermore, I agree that the changes that are made do clarify the structure of the code better than the old indentation did.
Combining all that I'm happy to merge this with the new indentation in the React files.

[rbergen]

@KeiranHines Just noting for the record that due to the magnitude of the change, I'll test this on the two devices I have available to me (Mesmerizer and M5StickC Plus) before moving forward towards merging.

[KeiranHines]

No worries @rbergen I expected someone would test it out. I have only noticed a few slight graphical changes. mostly stemming from css that was correctly applied previously. If you notice anything you'd consider unacceptable please let me know.

[rbergen]

@KeiranHines At this moment the main criterion is that we don't suffer any real functional regression. When the foundations for future development of the web UI are in place and you (and possibly others) start working on the web UI work items in the Issues list, then we can start being pickier about the "graphical details".

I know Dave has stronger opinions about the appearance side of things than I do, but he's also not a proponent of optimizing too early. To a certain extent it's up to you (plural you, as in the web UI developers) to indicate when you need either:

1. prior input about graphical (or in fact, functional) affairs before starting work on the aforementioned work items.
2. a "fine-toothed comb" review on the graphics that you've created.

However, first things first. I'll try my best to test this PR in my evening (it's 3PM for me when I write this). I'll let you know if I find anything that looks "out of order".

[rbergen]

Ok, I've tested this on both devices, and the web UI seems to work well. The persistence of configuration and theme works as advertized, and everything else that used to still does too.

I did notice one thing, which is that package-lock.json seems to insist on wanting to lose the empty "requires" object in the "babel/plugin-proposal-private-property-in-object" dependency. That makes that git keeps flagging package-lock.json as a modified file that should be checked in. It doesn't break anything, but it is somewhat annoying.

Could that be because I'm using a different version of npm (mine is 9.6.7) than you did while developing?

[KeiranHines]

@rbergen You are correct with package-lock. I have updated the repo now to use npm ci instead of npm install for baking the site. npm ci should maintain the existing package-lock regardless of version. Please let me know if that works for you.

[rbergen]

@KeiranHines Yes, that seems to have done the trick, thanks! I took the liberty of updating one comment in bake_site.py - the original version looked like an "unedited copy/paste" to me. If you're happy with that then I'm happy to merge this PR.

[KeiranHines]

Thanks @rbergen I am happy with that change.

[rbergen]

@KeiranHines Thank you very much for taking up the web UI challenge, and getting its development moving again!