-
Notifications
You must be signed in to change notification settings - Fork 666
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
Solr 12276 angularjs to angular migration #2818
base: main
Are you sure you want to change the base?
Solr 12276 angularjs to angular migration #2818
Conversation
JFYI: About 200 or more changes in files are moving to separate directory old kind of application and migration of images to new application. So actual changed files are less the 100 or even fewer. Please take into account, for other components the PRs won't be huge like this one. This happened just because setting up completely new framework from scratches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all I want to thank you for the time and effort you have put together into this PR and migration. It is nice to have options to choose from and to compare with one another for decisions of this scale.
I have a few questions in regards to the changes included that would be great if you can answer them:
- What was the reason again you had to rename / move the initial webapp module?
- Have you looked into the file size of a deployment? This would be an interesting value to compare with other framework if you have it available.
- The current state of the PR seems to not integrate the Angular project into the current "deployment", resulting in CORS issues when running with
npm start
. Do you have any instructions on how to run it so that we can see the UI in action?
A short feedback about improving the quality of your PR: If it would be possible to keep the current webapp untouched, the PR would be much smaller and easier to review. The lines of code would also better represent the actual work and part that should be reviewed. Seeing 55k lines of code "scares" many reviewers, even if it is just generated or moved stuff.
Right now the files relevant for review are probably less than 60 files, all located in currentUi/solrUi
.
I would also like to have a documentation that provide resources for further reading or basic ideas behind things like project structure (of the frontend of course). I think this would give our current community an easier start for both reviewing and working with the code.
Starting and testing the current UI seems also not to be working in this PR, which would be necessary for other reviewers to form an opinion.
Note: From here on, many of the things I mention are based on my own opinions and experiences.
From what I remember, in your migration you try to migrate the entire UI as is to Angular and then introduce changes in design and features. From a general perspective, I would follow this approach 90% of the times, as it quicker, less error-prone and often comes with test migrations, guaranteeing the functionality of the new code base. For our case however I believe that it is not worth the effort to study the existing code and try mitigating a behavior that is probably strongly outdated. At the time of writing of the UI I believe many concepts were not established and therefore many parts of the code may be implemented differently nowadays. I would therefore consider reevaluating the use cases / features to only "migrate" those useful to the users and directly implement them as it is expected by the framework.
About the actual code
What I saw and liked is that you included pipes, which is one of the ways Angular is processing and transforming data. This allows other reviewers unfamiliar with Angular to see some of its features.
What I also like and is included is the use of environments that allow the project to be built for development or production. We are already considering introducing such concepts in places like the CLI.
What I would probably do different is using Typescript optionals (like in value?: type
) wherever possible and necessary, rather than relying on undefined (like in
value: type | undefined`).
I'd also like to follow naming conventions and avoid mixing things. I saw that some files and folders are following the kebab-case with dot-notation, other use the PascalCase and some even use camelCase.
I am not sure if the styles you used are based on a UI kit, but I recommend using one as a library. This would require less customization with CSS, reducing the amount of code that needs maintenance even further.
When it comes to style values, I strongly encourage you to make use of the 8-point grid system.
I've also seen that each component's SCSS defines its own colors at the top. I would avoid that because it makes it harder to later update the look and feel. You would have to change multiple places for adjusting these values. Since you are using SCSS it shouldn't be a problem importing variables from another file.
What would also simplify and reduce the stylesheets, is to make heavier use of inline styling for styles that are applied to single elements and not groups / classes. For example, attributes like background-image
could inline-styles of the element or be added as separate DOM elements rather than styles.
To reduce the influence of changing APIs, I would avoid using the same data entities from the API in the view. Introducing an abstraction layer and mapping the values would reduce the impact of API changes to only the services.
To avoid "repetitive" code in components, I would prefer using data binding. For simple getters like in main.component.ts
one-way data binding, from source to view, would be more than sufficient and reduce a lot the sizes of components. And in combination with the previous recommendation of abstracting the API data classes from the rest of the app, it would also be safe to directly use the attributes from the component (as these values would be read-only in the UI).
About Angular
What I personally like about Angular is that it is based on Typescript and well defined. It provides everything you need in a web frontend and has great documentation. What I do not like is the use of annotations and generated code. This often results in unknown behavior and outcomes, sometimes even limitations.
Personally I am not a big fan of HTML and CSS. I have spent way too many hours with trying to position and size things correctly. The same styles applied to an element strongly depends on its parents' style, and therefore may look completely different if reused.
Now about the main topic here,
Using Angular for the Admin UI migration
Angular is a great framework that provides a great documentation. Like any other framework, Angular requires time to learn, and because it makes heavy use of annotations and uses a lot of design patterns, it will take more time to understand what is happening in the background. It is not difficult for our use case to learn, it only needs a more time compared to other frameworks.
Because Angular is a framework, the project structure is given and well defined with various patterns. Therefore, we do not need to be creative in that part and can simply follow the instructions / documentation of Angular.
When it comes to compatibility with previous source code, Angular does not provide any benefit over any other framework. AngularJS is a thing on its own and no matter what framework we would use for the migration, we still have to migrate the entire UI into a new codebase.
Reusing any of the existing code will be difficult and migrating it may be more error-prone than simply writing new fresh code based on the framework in use. I prefer bugs in code I have newly written / reviewed, rather than bugs in code I have altered and tried to match legacy behavior. In my opinion, we should use the existing code only for understanding what the UI was doing before.
Looking into our current challenges, one of the main problems is the maintenance of the current webapp. The current community is less experienced in frontend development and web technologies. This has proven to be a problem when it comes to maintain the old and new codebase. Therefore, taking the community's preferences is crucial for the long-term livability of the frontend.
Another point that should not be taken lightly here, is that an Angular frontend (and any other web-based framework) comes with its own package manager (NPM), languages (Javascript, Typescript, HTML) and stylesheets (CSS). Developers would need to familiarize themselves with new technologies besides the ones already in use. Integrating a web-based frontend into our current project is possible (already proven), but multiple workarounds and customizations are necessary. Choosing a technology that is based on the current stack (Gradle, JVM) would be beneficial for the community, simplify processes and reduce the amount of customizations / workarounds.
I believe that if we would want a desktop client in the future, we would need to make use of third-party solutions like Electron.
A few less important notes about the content of the PR
The below notes can be ignored and will become relevant only in case this PR will be chosen:
Multiple license headers have been altered
This should not happen for files that are not our own or generated. I saw that multiple CSS files and js files from the libraries have modified license headers. This probably happened while moving things around.
License headers added to or missing from some files
In HTML if I remember correct we avoided adding license headers due to the file sizes and additional traffic needed. If we plan to add them, we should make consider stripping them off from individual files and provide a single license in the packaged webapp (if that is an acceptable approach).
References to assets seems problematic
I believe that references of the type ../../../solrUi/src/assets/*
are likely to break during deployment, as the project is being built and moved to different directories. If I am not mistaken, the src folder does also not exist as such once the project is built. I believe the correct reference would be just /assets/*
. This may happend unintentionally while moving things around. Pointing that out in case we decide to follow this approach.
I have left some inline notes as well, sometimes repeating what I said above just for reference.
Offtopic
There has been a discussion in the Compose PR about assets and their representation. Luckily, the new UI designs focus on vector icons and do not use PNGs for assets, like it was done before. This is in my opinion very beneficial from the theming and maintenance perspectives. We should therefore avoid reusing the existing assets in a new UI.
I'd also like to see types and interfaces from our OpenAPI spec being generated and used in web projects, once available.
A web-based solution would probably require us to find a solution for integrating license / notice file checks related to the dependencies. I am not sure how this is handled right now, but we will probably require some checking to be done for all the packages loaded by web-projects. And we may end up with a custom solution if we can't find something that integrates well with our current stack. This topic is worth its own thread though.
solr/webapp/web/solrUi/src/app/modules/dashboard/models/systeminfo/ILuceneInformation.ts
Show resolved
Hide resolved
solr/webapp/web/solrUi/src/app/modules/dashboard/components/main/main.component.html
Show resolved
Hide resolved
@NgModule({ | ||
declarations: [SizeUnitConverterPipe, TimeAgoPipe], | ||
imports: [CommonModule, NgClass, NgFor, NgIf], | ||
exports: [SizeUnitConverterPipe, TimeAgoPipe, NgClass, NgFor, NgIf], | ||
}) | ||
export class SharedModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this doesn't have to be an angular module / component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of modularity that Eric has talked. That other extensions can be applied on the fly, this is just one of the first step to provide common logic across multiple components and modules within Angular, as well in the future there will Interface/Adapter implemented to support external modules integration to the application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I saw only the static functions in the module and thought "if all its content is static, what is the role of this class to be a module?". I didn't have a context where this module could be used.
It is nice that you thought about extendability of the frontend, I was asked about this too. There are definitely multiple ways to achieve this, but I believe integrating it too early may overcomplicate things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malliaridis, well If we begin our implementation and only at the later stage add the structure to integrate third party extension, it might require significant amount of work at the latest stage of the development.
It's simple for example, if we want to integrate multi-laguage support into our UI, it would be easier to add it at this stage of the application development rather at the latest stage.
Regarding this Shared module, is more about code grouping within the application structure.
At this point of the implementation, we have only one component is provided for review.
No problem, this is just an experience for me and my friends.
Let's start from the first question:
Okay, I'll try to revert most of the changes related to the current solution back that it won't affect PR.
Well, I'm not sure what do you mean under the documentation with this. If you're speaking about the documentation such as README file then I will add one how to start local environment, until we'll have complete integration with backend solution, it won't help. I hope I correctly understood you.
If you're speaking about unit/integration test, first of all I'm planning to move all the current functionality that exists at this moment toward to the new implementation that it went smoothly, and we can simply continue add features and refactor the current solution. If you're speaking about the way how to start and poke review it. Then I do everything from |
I won't be so worry about that, this is just for source while webpack that runs under the hood of the ng build, packs all that requires to assets of the apllication here is an example:
I took this line from the compiled and minimized css file that appeared after compilation. |
2560326
to
0c7de12
Compare
Thanks. So that would still be an open TODO in case we decide to proceed with this one.
This now way easier to review, thanks a lot.
Yes, what I (and probably others as well) would like to see is how it would integrate into the current project (which is as described above an open TODO), how someone would work and test the code base (like running the UI with
I meant starting and poking around, yes. So there is no state yet that makes request to a Solr backend, is that correct? I think you removed a few too manyfiles in your latest updates. The environment files under And one last tip, it's fine and also recommended to include |
https://issues.apache.org/jira/browse/SOLR-12276
Description
The fully simple migration to the newest version of Angular application which is Angular 18.x at this moment.
The basic idea of this migration it's not only legacy codebase migration and also support the modularity of the application for some third-party extensions. Migration from JavaScript to TypeScript opens and opportunity for in some sort of the Strong typed development which gives more opportunity to avoid some shadowed changes or assignment. As well as implement Interface - Adapter which allow to implement standalone components and combined with main application.
Solution
This is a draft version for the New codebase of Solr Ui application. In this PR there are only Fully functioned a Dashboard
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.