search

keycloakify / keycloakify-starter-angular

🔑 Keycloakify Angular Projects Starter Template for Webpack
6 stars 1 forks source link

Project Refactoring #4

Closed luca-peruzzo closed 1 month ago

luca-peruzzo commented 1 month ago

Hi! I've been working with angular for many years and I'm considering implementing keycloakify in a project I follow, so I came across this starter template. I saw that it's in early stage for the time being, so I'd like to help move the project forward by providing my knowledge. I have been refactoring the code to try to make it as close to angular 18 standards as possible. If you'd like to try looking at this PR and let me know if you need a hand, I'll leave some comments on the code so we can discuss it!

luca-peruzzo commented 1 month ago

@kathari00 I've fixed all the conflicts :)

luca-peruzzo commented 1 month ago

@kathari00 sorry I only just realised the plugin I deleted. I restored it by adapting it for esbuild, try to see if it works like the previous one

kathari00 commented 1 month ago

Hi again,

I merged it BUT everything is broken. The desired build structure that keycloakify needs is not fullfilled at all. I fixed that by going back to custom-webpack. Why should that not be used the package is very popular compared to the esbuild one. Is this a Angular 17 thing? There is still stuff that is not working like the navigation.

Have you tried running the application before opening the PR? For ng serve and with the server?

luca-peruzzo commented 1 month ago

Hi again,

I merged it BUT everything is broken. The desired build structure that keycloakify needs is not fullfilled at all. I fixed that by going back to custom-webpack. Why should that not be used the package is very popular compared to the esbuild one. Is this a Angular 17 thing? There is still stuff that is not working like the navigation.

Have you tried running the application before opening the PR? For ng serve and with the server?

Of course i have tried to build and run the app... In development with the mock and without styles in angular json it works. There are differences in keycloakify versions in how it builds assets (there's a path change). For the build output format of course you can go back with webpack (remember to use esm) or we could try to fix the esbuild plugin. Probably Monday I can fix those bugs

garronej commented 1 month ago

Hi, I’m following this from a distance, but I wanted to point out something that may be helpful. The Keycloakify compiler only supports Rollup (only in Vite projects, Vite uses Rollup to generate the production build) and Webpack.

I’m not sure if this is relevant to your PR, but if the production build is being generated using Esbuild, it won’t work in production.

kathari00 commented 1 month ago

I would prefer to use my working solution as @garronej pointed out it is not possible to use Esbuild. I don't know what you mean by the assets. The url is generated at runtime so of course it changes. I like your intention to help but I don't like to discuss things that are not working like the navigation or the build. I would prefer you test your solution first and then we can discuss it.

I like your refactorings on the Angular 18 relating stuff, I learned a lot.

luca-peruzzo commented 1 month ago

I would prefer to use my working solution as @garronej pointed out it is not possible to use Esbuild. I don't know what you mean by the assets. The url is generated at runtime so of course it changes. I like your intention to help but I don't like to discuss things that are not working like the navigation or the build. I would prefer you test your solution first and then we can discuss it.

I like your refactorings on the Angular 18 relating stuff, I learned a lot.

I mean the assets generated by keycloakify (patternfly and other stuff). As mentioned in the previous commit i will fix probably Monday

kathari00 commented 1 month ago

Okay. Make sure to get the recent master and test your changes beforehand.