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

Platform UI - React integration #5011

Merged
merged 73 commits into from
Jul 18, 2023
Merged

Conversation

matmair
Copy link
Member

@matmair matmair commented Jun 9, 2023

This PR introduces the base for the new UI based on React. The current UI is referred 'classic' while the new React one is named 'platform' - as it is based on a new UI platform.
The UI itself is de facto empty - it is just a shell to show how the mechanism works. There will be a parallel PR with a first basic interface.

Detailed mechanisms:

  • there are env/config.yaml mechanisms to select which UI urls should be served
  • there are invoke tasks to build the code for the frontend and collect it into static
  • there are yarn tasks to extract/compile language files (based on gettext)

Missing:

  • dev docs
  • min version of node. technically 16 but I would like to set a higher one (19/20) from the start
  • build step for docker (multistep - to not include node on the image) maybe after Refactor: Dockerimage #5007
  • build step for releases
  • merging of translation files (or mapping - still not sure about that)

Fixes #3901 Fixes #2789

@matmair matmair added this to the 1.0.0 milestone Jun 9, 2023
@matmair matmair self-assigned this Jun 9, 2023
@matmair matmair marked this pull request as draft June 9, 2023 22:05
InvenTree/web/urls.py Show resolved Hide resolved
src/frontend/public/inventree.svg Outdated Show resolved Hide resolved
@SchrodingersGat
Copy link
Member

Exciting times!

Regarding translation - we will need to be careful that during this transition process we don't delete any existing translated strings (as much as we possibly can). Otherwise we lose all the great work that has been done on crowdin.

@SchrodingersGat
Copy link
Member

@matmair when this is ready to look at, can you provide a set of instructions to build and run a MVP?

@matmair matmair mentioned this pull request Jun 11, 2023
2 tasks
@SchrodingersGat
Copy link
Member

@matmair I'd like to wait for response to @wolflu05 questions above :)

@matmair
Copy link
Member Author

matmair commented Jul 11, 2023

@wolflu05

  1. they are required to check if all parts of the UI are translateable
  2. if does not make sense to split out one language, will just lead to inconsistencies
  3. yes because the compiled static need to be included to run without node

Those files should be committed - I just removed them for this PR because the UI is empty rn and it makes reading the diff harder without adding any benefit. It is the same as with the language files - we automated that part but at some point they need to be updated and included.

@matmair
Copy link
Member Author

matmair commented Jul 11, 2023

I would appreciate it if you would not extend your content meaningful after commenting by editing - I mainly read on mails to decide if I need to answer and do not get updates for edits @wolflu05

@wolflu05
Copy link
Contributor

Those files should be committed - I just removed them for this PR because the UI is empty rn and it makes reading the diff harder without adding any benefit. It is the same as with the language files - we automated that part but at some point they need to be updated and included.

I don't like commiting the build output. As you said, the diff gets really huge, that means also the repo size rapidly grows => it takes an eternity to clone the repo, currently it already needs 40s to clone. I would say that node is required if you develop frontend parts and if you don't like to install it, use the devcontainer. It's a really bad idea if this is the only argument to commit these huge diffs later.

And regarding the languages and pseudo languages, you plan to commit them before merge?

@SchrodingersGat
Copy link
Member

With regards to translation files:

  • We need to be very careful that we don't remove the current string translations which have been done on crowdin
  • Any new translations (i.e. in react) should match the strings used in existing translations
  • We need to very carefully ensure that adding in new translation files happens in a way which is compatible with our current translation setup
  • At some point, we'll remove translation for html templates and js templates (way down the track)

@matmair with this in mind, can you please briefly outline how the translation files are calculated / stored in the new framework? So we can determine how we plan for integration with crowdin.

@matmair
Copy link
Member Author

matmair commented Jul 12, 2023

@wolflu05 if you see another way to distribute the front end I am open to ideas. The alternative way still has to work for bare metal installs - we would brake the upgrade path for most users otherwise judging from the issues.

@matmair
Copy link
Member Author

matmair commented Jul 12, 2023

@SchrodingersGat translations work basically the same as they do with the current frontend. Strings are discovered when the frontend-trans task is run, placed in .po files, those are picked up by crowdin, translated, pushed from crowdin to the translation branch and then merged into master before release.

  • We need to be very careful that we don't remove the current string translations which have been done on crowdin

The new translations would be separate, like the app translations

  • Any new translations (i.e. in react) should match the strings used in existing translations

Not sure how much this will be possible - a lot of the interface is underdocumented so there will probably be a lot of new tooltips, menu entries etc.

  • We need to very carefully ensure that adding in new translation files happens in a way which is compatible with our current translation setup

Uses the same mechanism as the current setup

  • At some point, we'll remove translation for html templates and js templates (way down the track)

A lot of strings are also used by the backend so that will not remove a lot of translatable files - a bunch of glue pieces for the current way of translations will be removed tough

@SchrodingersGat
Copy link
Member

@matmair ok thanks, I'm happy with that. Sounds like we have a way forward at least

@SchrodingersGat
Copy link
Member

Anything holding us back from merging at this point?

@wolflu05
Copy link
Contributor

wolflu05 commented Jul 12, 2023

if you see another way to distribute the front end I am open to ideas. The alternative way still has to work for bare metal installs - we would brake the upgrade path for most users otherwise judging from the issues.

We can build them and upload a zip file with the point releases. If you want to install from a none point release, require node. I know a lot of softwares that require node for their frontend build (e.g. gitea). And for apt packages, I guess there is already some build step where this could be build.

@wolflu05
Copy link
Contributor

Anything holding us back from merging at this point?

Do you like the idea of commiting build files? I already explained a lot of downsides here

@SchrodingersGat
Copy link
Member

SchrodingersGat commented Jul 12, 2023

My feeling is that the compiled web files should not be included in the repo. If we run through the different ways to use InvenTree:

Setup Method How to get src files
Docker Compiled as part of docker image build
Installer Compiled as part of install process
Bare Metal Compiled as part of install procedure
Devcontainer Probably not needed, run using yarn?
Other ... ???

@matmair
Copy link
Member Author

matmair commented Jul 13, 2023

Compiling the frontend on the server with bare metal and installer means that every user needs to install thousands of external packages on their machine. If only one of those is vulnerable (pretty likely) web proxies or on-device scanners will trigger - effectively making inventree uninstallable in the enterprise/goverment or for responsible sysadmins.
If this is the plan I need to hand-break my involvement in this whole thing hard as all usability in my deployments is gone. Requiring nodejs on the server means no installation in my environments - the whole frontend integration is designed around this requirement. If the user installs nodejs you could just use the build output of that but at that point, I can not deploy anymore. Also, the installation footprint shoots up significantly if you need all the modules on the server - that is above 1gb of unneeded junk for a 5-10 mb frontend.

@wolflu05
Copy link
Contributor

Why not upload a build output to each point release as a zip file?

@SchrodingersGat
Copy link
Member

Compiling the frontend on the server with bare metal and installer means that every user needs to install thousands of external packages on their machine. If only one of those is vulnerable (pretty likely) web proxies or on-device scanners will trigger - effectively making inventree uninstallable in the enterprise/goverment or for responsible sysadmins.
If this is the plan I need to hand-break my involvement in this whole thing hard as all usability in my deployments is gone. Requiring nodejs on the server means no installation in my environments - the whole frontend integration is designed around this requirement. If the user installs nodejs you could just use the build output of that but at that point, I can not deploy anymore. Also, the installation footprint shoots up significantly if you need all the modules on the server - that is above 1gb of unneeded junk for a 5-10 mb frontend.

You make some great points here - the security implications in particular I had not considered.

I had been considering this front-end code in a similar way to how we currently render static files with invoke static. But if we are not going to be shipping node at all, as it is not required to actually serve the compiled frontend code, then this makes sense.

I'm happy to move forward with compiled front-end files "baked in". Let's keep this going, this is a super important path forward for the project.

@wolflu05
Copy link
Contributor

wolflu05 commented Jul 13, 2023

I see the points with the vulnerabilities, but really, we shouldn't commit the build output. There are a lot of downsides:

  1. The repo contains 14.5k commits and the folder is huge, 209MB currently. Cloning takes about 40s on my machine. If we commit the build files we dramatically increase that time and also the size.
  2. PRs contain huge diffs of build changes
  3. The build output is not very consistent, if you change only a few components it can result in a changed build output from ground up, because e.g. you have different versions of node packages. The build output files are normally minified, so they are all in one line
  4. Merging get a PITA, especially with multiple concurrent merges as every pr changes these files

How to properly solve this problem:

As stated by @SchrodingersGat we have a few targets. My suggesting would be the following:

We have to distinguish between development and production.

Development

I think for development on the frontend it's not a big deal to install nodejs, you need it anyway. If you only want to do backend, go ahead and use some API client like postman or the integrated one. For devcontainer, its not a big deal the devcontainer contains node anyway.

Production

Setup method How to deliver the static build output
Docker ✅ Compiled as part of docker image multistage build
Installer ❓ Compiled as part of the deb package build process
Bare Metal ❓ Compiled as part of a release pipeline and uploaded as a zip folder to the GitHub releases. The setup instructions could then be: just download the zip folder, extract it and place it inside static
Bare Metal (no point release) ❓ Can't help you, you need node, or at least docker and build it yourself, very small user base and also not the recommended setup for production

Please, evaluate the downsides, they are big for me. I really don't like to have them in here. I really know we want to move this forward, but once we decide to commit the repo will definitely grow fast and we can't reverse it.

@SchrodingersGat
Copy link
Member

Maybe due to me being a complete react noob, but is it possible you guys are talking about different things?

@wolflu05 you are talking about 200mb worth of files - which directory, specifically, and what purpose do these files serve?

@matmair you are talking about 10mb worth of files - which directory, specifically, and what purpose do these files serve?

@wolflu05
Copy link
Contributor

wolflu05 commented Jul 13, 2023

I'm talking about the files in InvenTree/web/static/ that are generated by invoke frontend-build. They are only 5-20MB even the project grows, but committing a diff in those files only a couple of times increases the repo size, commit diff, checkout time by a lot.

@matmair matmair mentioned this pull request Jul 13, 2023
2 tasks
@matmair
Copy link
Member Author

matmair commented Jul 13, 2023

I would rather have devs download a bigger repo from time to time then most users (especially the ones that have been using this for years when bare metal was the only real deployment option) dealing with node. Manual changes to the update methodology are about the most brutal thing you can do to a sysadmin.

@SchrodingersGat I am talking about the real files that are acutally present

@wolflu05 with that argument we should stop translations

@matmair
Copy link
Member Author

matmair commented Jul 13, 2023

I will step back from discussions for a few days as I do not feel that it will be productive in the end. Favouring one deployment method because of the lack of the others will only lead to more support load and if that is the result I'd rather ship the frontend as a plugin or reconsider the rewrite.

@SchrodingersGat
Copy link
Member

@matmair @wolflu05 are we happy to move on from here, and merge this (without the compiled front-end files), with the understanding that this code will not go into production without a complete, robust method for distribution that does not require the user to compile from node?

@matmair
Copy link
Member Author

matmair commented Jul 18, 2023

fine with me @SchrodingersGat

@wolflu05
Copy link
Contributor

Yeah, also fine with me, merging this now definitely simplifies things e.g. the huge diff in the other PRs.

@SchrodingersGat
Copy link
Member

Fantastic :) @matmair are there any other changes you want to make here, or happy to merge as-is?

@matmair
Copy link
Member Author

matmair commented Jul 18, 2023

Yes @SchrodingersGat let's merge

@SchrodingersGat SchrodingersGat merged commit 3e37469 into inventree:master Jul 18, 2023
13 checks passed
@matmair matmair deleted the plattform branch July 23, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Epic: React [FR] Switch UI to React
3 participants