-
Notifications
You must be signed in to change notification settings - Fork 722
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
GUACAMOLE-1085: Migrate frontend to angular #896
base: next
Are you sure you want to change the base?
GUACAMOLE-1085: Migrate frontend to angular #896
Conversation
😮 Thanks, @leonard2901! |
If it should be done or needs to be done, then yes - absolutely. |
That would be great. Then it would be possible to also publish the Angular libraries to NPM and further simplify the creation of extensions and other applications. |
Any feedback is appreciated - not limited to Mike of course. If anyone gave it a try (build/use/look-into), please post your comments. |
"ng": "ng", | ||
"start": "ng serve", | ||
"build": "ng build", | ||
"build:app": "ng build guacamole-frontend --base-href /guacamole/", |
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.
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 will add this to the todo list and take a look at it.
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.
We internally discussed the issue with the the static base-href configuration. As Angular requires the base path to be set at compile time, dynamically adjusting it isn't straightforward.
Our suggestion would be to keep the --base-href guacamole
setup for now. However, as a workaround, users who need to deploy the webapp at a different base can manually adjust the base path: they would need to unpack the .war file, edit the base href in the index.html file to the desired path, and then repackage it.
I deployed the app at Super cool! |
Hi! Thanks for your feedback so far. I’m working on finishing the remaining todos, and I wanted to ask if, once they’re done, it’s realistic to aim for a merge. Appreciate your thoughts! |
I think so. It's quite a huge change, to be sure, and all the more reason to avoid too much long-term divergence. Moving to Angular from AngularJS will mean a full version bump to 2.0.0, but that's OK. We can figure out a branching scheme that won't make maintenance of 1.5.x, upcoming 1.6.0, etc. impossible. |
That sounds great. Currently guacamole-common-js is provided here as an npm package. With the transition to Angular and TypeScript, it would be nice if guacamole-common-js was offered as an "official" package at npm. @mike-jumper was already positive about this idea. |
Ah, yes - that guy. ;)
The build would need to be altered however necessary to produce that package and consume it. The current Maven-based process is:
An NPM-based process would need to be:
|
In addition to that: Perhaps it would be a good idea to reserve a good name as 'guacamole' as package seems to be taken https://www.npmjs.com/package/guacamole |
@everflux Sadly, it looks like the https://www.npmjs.com/org/guacamole As for the package itself, I think a reasonable path forward might be to get in contact with @padarom, the individual that produced and maintains an NPM-ified fork of guacamole-common-js: https://www.npmjs.com/package/guacamole-common-js If we can take over ownership and maintenance of that package within NPM, then existing users of the established NPM package would be able to continue using it without interruption, and former users of the original pure-Maven package looking to migrate to NPM would have the benefit of familiar package naming. @padarom, any thoughts? |
The intention of my fork was to make it easier for people to use Guacamole in their JS projects, so if there ends up being an official Apache package doing the same, then my fork can be seen as obsolete. As long as the official npm package is compatible, I'm fine with transferring ownership. If I understood correctly the plan seems to include a major version bump, so even if it isn't quite compatible some breakage could be expected by the package's current user base anyways. GitHub's diff viewer isn't being nice to me with this large PR and I'm currently on the go, but from the PR description it seems like the regular Guacamole frontend is rewritten in Typescript, whereas |
I agree. |
I see no immediate reason why my changes to the frontend should lead to breaking changes in the JS lib.
I also agree. However, to avoid making this PR even bigger, I would suggest that this should be done as a separate task after this PR has been merged |
@leonard2901: when you are planning to release your guacamole-client-angular as a npm package? |
@suncase Thank you for you interest in the Angular client. As you can see in the description of the PR, there are still a few things I need to work on before this could possibly be merged. Unfortunately, I currently have less time for the project than I would have liked. So I can't tell you when this will be merged. That being said, I don't think the frontend of the Guacamole Client will be released as an npm package. |
5e26039
to
25a1927
Compare
I’ve just updated the pull request with the latest progress. The migration should now be mostly complete. It would be great if you could take a look—any feedback is appreciated. Specifically, I’ve incorporated most of the recent changes from the main branch, rebased this branch onto main, updated the project to Angular 19, and extended the Java application to provide information about Angular extensions. Some time ago there was the idea of revisiting guacamole-common-js to add TypeScript types and publish it to npm. I think this would be a valuable step, but it's not strictly necessary for this migration. Given the already considerable scope of this PR, I'd suggest doing it as a separate task. |
I believe this should be based against the
Agree this is probably better for a separate PR, and a separate Jira issue. |
Angular Components introduce new elements which interfere with the display: table CSS rule. The display: contents rule is used to ignore these elements inside the css table.
- Remove only partially implemented support for copy/pasting images. - Remove workaround for missing clipboard API
- translations - clipboard - settings - player
- translations
25a1927
to
c637976
Compare
This PR includes the current progress for the migration of the AngularJS frontend to Angular.
There is still a lot of work left to do, but I think now would be a good time to get some feedback on the code.
Overview
The new frontend consists of one Angular app and two Angular libraries:
guacamole-frontend
guacamole-frontend-lib
guacamole-frontend-ext-lib
The application and both libraries are included in the maven build.
TODOs:
index/services/iconService.js
manage/directives/connectionPermissionEditor.js
manage/controllers/manageSharingProfileController.js
projects/guacamole-frontend-lib/src/lib/types/Guacamole.ts
). They will be replaced by the package @types/guacamole-common-js once it is updated.The angular build creates a
3rdpartylicenses.txt
file, but I don't really know what to do with it.Any comments on this will be appreciated.
Extensions
The biggest challenge in the migration was to build an extension system in Angular that was as flexible as the one in AngularJS.
apply-patches.service.ts
andstyle-loader.service.ts
in
projects/guacamole-frontend/src/app/index/services/
doc/guacamole-frontend-extension-example
for an example.Implementation notes
methods. Some of the more complex services are replaced by Angular services.
restrict: 'E'
,restrict: 'A'
.AuthenticationService.request()
)is replaced by a HTTP interceptor
(
auth/interceptor/authentication.interceptor.ts
).rest/services/requestService.js
) is replaced by a HTTP interceptor(
rest/interceptor/error-handling.interceptor.ts
).httpDefaults.js
) is replaced by an HTTPinterceptor (
index/config/default-headers.interceptor.ts
).InterceptorService
can be used.instead of the
@:
prefix. However, I change it to@:...:@
to prevent transcloco from removing parts of ICU messages (Bug(messageformat): Plural translation starting and ending with a variables fails jsverse/transloco#621).kept?
TunnelService~uploadToStream
TunnelService~downloadStream
UserCredentials.getLink
UserCredentialService~getLink
$parse
function of AngularJS I used the NPM package angular-expressionswhich is a copy of the AngularJS code as standalone module.
Running the code
For my setup the easiest way to run the code was to build the frontend with maven and use the war file in a docker-compose setup. Since the module federation config is hard coded for now there will be some errors when the example extension (
doc/guacamole-frontend-extension-example
) is not running (ng serve
) atlocalhost:4202
. If the extension is running, the content will be available at /extension-example.It is also possible to run the frontend using
ng serve
and benefit from live reloading with a way to work around the same origin policy (modify HTTP headers/proxy/disable web security). Therefore, the libraries need to be built manually:ng build guacamole-frontend-lib
andng build guacamole-frontend-ext-lib
. Additionally, some code changes are necessary to redirect the requests to the REST API. I created a patch for that: https://gist.github.com/leonard2901/93a81e77f0ec26e049481a7189791478When using
ng serve
, the REST API from this branch should be used.