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

Move project to React #5

Merged
merged 99 commits into from
May 7, 2024
Merged

Move project to React #5

merged 99 commits into from
May 7, 2024

Conversation

barrenechea
Copy link
Contributor

@barrenechea barrenechea commented Apr 29, 2024

This is an extensive pull request. If approved, feel free to squash it at the end since it comprises many commits 😄 huge thanks to @frankurk for helping me on the CSS side!

The goal is to move the project into React and change how production builds are distributed. I updated the README.md to detail the development process, how production builds work, and contribution restrictions (mainly build size).

jQuery -> React

The goal was to keep the original interface as untouched as possible while adding new features and not going wild on the build size.

  • Everything's built on TypeScript.
  • Pretty much everything is lazy loaded, meaning the app only loads the required assets for the current view. Lowers the time to first render.
  • Every view that loads data from the API now has a loading skeleton. It may not even be visible since the API replies quickly, but it's there.
  • Auth: I replaced Basic Auth with a login view and handed over the username/password combo for a Bearer token to be used in further API requests.
  • Routes: Implemented via Tanstack Router, it provides SpA routing & route protection.
  • Query/Mutation management via Tanstack Query: adds caching and mainly allows you to invalidate queries/mutations, refetching updated information as required.
  • I added a Fan control, a 404 page, and some tweaks here and there (like the Switch button on the Nodes view not having a differentiable disabled state, or the hostname not being replaced in the header).

Removing Basic Auth and the SpA handling routes requires a supporting PR over bmcd: turing-machines/bmcd#86

The current React bundle size is 1194kb.
The current jQuery project's dist folder is 1277kb.

Quality Checks

I added a quality.yml pipeline that does several basic checks for every upcoming pull request, such as:

  • Lint rules
  • Build the project
  • Check and report build size
  • Run tests (dummy at the moment)

Distribution process

Pushing the dist folder directly to the repo is not recommended, although I understood the usefulness of $(call github,...) in the Makefiles.

Leveraging GitHub Releases with version tracking and automated builds would solve the issue of having fixed static builds and predictable URLs to get them.
I added the tag-and-build.yml workflow to leverage the build and release according to the package.json's version field.

An example release can be found here.
I'll open a supporting PR over BMC-Firmware to download the file from GitHub Releases. turing-machines/BMC-Firmware#198

Screenshots (running on my cluster 😜) /login view Screenshot 2024-04-28 at 8 05 14 PM /info view Screenshot 2024-04-28 at 8 06 24 PM /nodes view Screenshot 2024-04-28 at 8 08 26 PM /usb view Screenshot 2024-04-28 at 8 23 04 PM /firmware-upgrade view Screenshot 2024-04-28 at 8 23 43 PM /flash-node view Screenshot 2024-04-28 at 8 24 16 PM /about view Screenshot 2024-04-28 at 8 24 50 PM Any non-existent client route (excludes /api/bmc routes) Screenshot 2024-04-28 at 8 10 30 PM

Fixes #4

@barrenechea
Copy link
Contributor Author

A few things on top of my mind:

  • The footer © TURING MACHINES INC. was copy/pasted from www.turingpi.com since I thought it looked pretty cool. I’m not sure about the © part. Does it conflict with the repo license or doesn’t really matter? Maybe a question for Legal 😆
  • I wanted to add a dynamic page title for each route, and even though there are libraries to do that (react-helmet, react-helmet-async), a first-party solution has been promised. I’d prefer to wait for it, we’re not in a hurry, really.
  • Modals had to be simplified a bit, mainly removing the animated stuff from SweetAlert. They look a bit more professional IMO now.
  • Form validation is pending, I wanted to add react-hook-form but I’d prefer to add it in a separate PR. This is big enough as it is.
  • If allowed to continue, after form validation, i’d like to tackle i18n, and hopefully dark mode at some point.

Comment on lines +16 to +18
1. Set up `bmcd-api-mock` for development:
- Clone the [bmcd-api-mock](https://github.com/barrenechea/bmcd-api-mock) repository.
- Follow the instructions in the `bmcd-api-mock` repository to set up and run the mock server.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I should leave this in. It helped me develop the new app, but it's not a "first-party solution" from Turing. Open to suggestions!

I guess an option would be for the dev to target its own cluster. I'm not sure if CORS restrictions apply there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, I did something similar with enabling CORS and, therefore, running the web page on my local machine, passing the API calls to the Turing board.

It could be useful, not sure if you are willing to donate it. But it would at least be worthy of a comment about its existence in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to donate the code, although I'd like to polish it a bit, like making it more modular and more "to-the-spec", specifically on error handling 😄

@svenrademakers
Copy link
Collaborator

@barrenechea That's really impressive! I'll take it for a test spin over the next couple of days. If it looks good, we can merge it as far as I'm concerned.

@svenrademakers
Copy link
Collaborator

svenrademakers commented Apr 29, 2024

The footer © TURING MACHINES INC. was copy/pasted from www.turingpi.com since I thought it looked pretty cool. I’m not sure about the © part. Does it conflict with the repo license or doesn’t really matter? Maybe a question for Legal 😆

that is perfectly fine

@svenrademakers
Copy link
Collaborator

@barrenechea been testing it a bit, and it is an improvement over our last UI. I'm happy to merge it. I however have 2 small comments:

  1. 'fan control' slider is not working.
  2. i get these fault notifications, however the 'get' calls under the save button gives me a 200 OK back?
    fancontrol
    err

@barrenechea
Copy link
Contributor Author

barrenechea commented May 6, 2024

@barrenechea been testing it a bit, and it is an improvement over our last UI. I'm happy to merge it. I however have 2 small comments:

  1. 'fan control' slider is not working.
  2. i get these fault notifications, however the 'get' calls under the save button gives me a 200 OK back?
    fancontrol
    err

Thank you for checking it out @svenrademakers !

Regarding fan control, it should only show up if at least one fan is detected (it should never render empty). Do you have the fan chip soldered in? I'd appreciate it if you could give me the answer you're getting for https://turingpi.local/api/bmc?opt=get&type=cooling as it may differ from mine. Given the screenshot, I'd definitely need to fix the "empty state".

And the node information may be my mock API being out-of-spec on the set call. I confirmed that the same thing happens to me on my cluster, I'll fix it today! 🚀

@barrenechea
Copy link
Contributor Author

The issue with node_info was that the official bmcd response is just HTTP 200 and no body, while my mock was returning response.data.response[0].result as ok. The app was trying to get the ok from the body and was failing. Setting it to return void in the app should fix it.

I'll give it a test and push it to this branch if it fixes it 😄

@svenrademakers
Copy link
Collaborator

Do you have the fan chip soldered in?

Im using a v2.5.1. which uses a PWM signal to directly to drive the FAN. (this driver is however not fixed yet). From a linux perspective you can assume the thermal driver is not loaded. Perhaps in this case we want to disable the whole fan control ?

the answer you're getting for https://turingpi.local/api/bmc?opt=get&type=cooling

curl "https://turingpi.local/api/bmc?opt=get&type=cooling" --header "Authorization: Basic cm9vdDp0dXJpbmc=" -k
{"response":[{"result":[]}]}⏎

@barrenechea
Copy link
Contributor Author

Do you have the fan chip soldered in?

Im using a v2.5.1. which uses a PWM signal to directly to drive the FAN. (this driver is however not fixed yet). From a linux perspective you can assume the thermal driver is not loaded. Perhaps in this case we want to disable the whole fan control ?

the answer you're getting for https://turingpi.local/api/bmc?opt=get&type=cooling

curl "https://turingpi.local/api/bmc?opt=get&type=cooling" --header "Authorization: Basic cm9vdDp0dXJpbmc=" -k
{"response":[{"result":[]}]}⏎

Got it! It was a quick fix. I was validating for a truthy value to render that area (and it seems an empty array is truthy 😆). It gets fixed if I explicitly check for coolingDevices.length > 0

c57f70c

@barrenechea
Copy link
Contributor Author

barrenechea commented May 6, 2024

I'm currently building a firmware with the latest changes, the artifact should be finished in an hour or so https://github.com/barrenechea/BMC-Firmware/actions/runs/8940379219

EDIT: I noticed the DTS files for v2.5.1 are not merged yet, my build may not work for the new board 😞

@svenrademakers svenrademakers merged commit b642c9e into turing-machines:main May 7, 2024
@svenrademakers
Copy link
Collaborator

Everything ok now! thanks for all your effort and patience :)

@svenrademakers
Copy link
Collaborator

created tag v3.0.0 for it

@barrenechea
Copy link
Contributor Author

created tag v3.0.0 for it

I noticed there is no GitHub Release for v3.0.0, with the new pipeline triggered for v2.1.0, but it seems the GitHub release was deleted.

The pipeline checks for the package.json version and if the tag does not exist for the current version, it tags and builds a release (which is pushed to GitHub releases).

I see two options:

  • Delete the v3.0.0 tag, and then update the package.json version field to 3.0.0. On push to main the pipeline will re-tag it (and most importantly, generate the GitHub release with the /dist files)
  • To update the workflow so that it triggers on-tag instead of push to main to generate the GitHub release.

Let me know what you think!

@barrenechea
Copy link
Contributor Author

barrenechea commented May 7, 2024

Oh, by the way, whatever is set on package.json's version field will be injected as BMC-UI Version for the /about tab. If package.json says v2.1.0 and the set tag is v3.0.0, the About tab will still say v2.1.0 because that's what package.json had during the build time.

Of course, the app and pipeline can be adjusted to fetch the tag during build time (probably as an env var) and use that instead of package.json to embed the correct version.

@svenrademakers
Copy link
Collaborator

Ah, I forgot that you also updated the GitHub pipeline. I like how you automated it.

I'm inclined to change it to an on-tag trigger. I like accumulating a couple of commits on master before we promote it to a release. (We could inject the version into the package.json in this case.)
The downside is that intermittent commits are harder to test as they are not released and, therefore, cannot be downloaded into the BMC-Firmware.

However...
There are no constraints on increasing the frequency of GitHub releases. Therefore, my prior argument might simply serve to satisfy my personal preferences regarding the workflow. lets go for option 1

@barrenechea
Copy link
Contributor Author

Everything looks perfect now! I'll update my PR over bmc-firmware now to match v3.0.0 and we're good to go 🚀

Thank you very much for your support on this!

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 this pull request may close these issues.

Refactor into React
2 participants