Review the JHipster library

jdubois commented 7 years ago

As discussed in #4662 I created a JHipster library, that is available here.

It is currently in version 0.0.2, and uploaded on Maven Central (it hasn't sync yet, it should be there in 2 hours).

I have refactored the generator to use that library (don't expect Travis to work on the first run, as the library isn't on Maven Central yet), and I would like to have your review on it:

I only "extracted" obvious code, that I don't think people modify
I also "extracted" the Graphite and Prometheus components, so I could remove their dependencies, to make the normal Jar lighter. I have added 2 Maven profiles to add them back easily (I still need to learn how to do this with Gradle!!), using conditionnal Beans (like Spring Boot does all the time)
Main issue for me is that I had to refactor the JHipsterProperties, so now we have a small JHipsterProperties in the JHipster library, and a ApplicationProperties with nearly all of the former configuration, still in the final generated application

The code is available in the https://github.com/jhipster/generator-jhipster/tree/migration-to-jhipster-library branch, please have a look and comment!

gzsombor commented 7 years ago

For a library, you can't mix the functionality with the policy - for a pure, generated solution it was fine, because you can freely modify the default policy, and live with that, but with this hardwired policy in a jar file, everything is getting more difficult, for example :

LoggingAspect - currently as the aspect is defined https://github.com/jhipster/jhipster/blob/master/src/main/java/io/github/jhipster/aop/logging/LoggingAspect.java#L36
- It will only log from io.gihtub.jhipster.store.repository - funny typo - and not for the user's generated services/repositories/etc
- Interestingly I had to customize this class before, because as every call logged into the same channel, we couldn't control which place is interesting, and which isn't, so we had add annotations to service methods, which shouldn't be logged, because the amount of noise. The other problem was some proxy generation issue - spring had some issues with creating proxies and injecting that implementation into various services. I couldn't recall the details unfortunately, but the quick solution was to restrict that aspect to a different package.
AsyncSpringLiquibase - this would be much better, if this service had only one flag : "sync/async" execution, and the client code could decide, based on his current knowledge which is preferred - the generated code could decide based on the current profiles/etc.

jdubois commented 7 years ago

Thanks @gzsombor

The "JHipster store" is my usual demo app, and yes it had a typo on gihtub for a long time, but that's just a demo :-)
OK the LoggingAspect shouldn't be in the lib, as this is something you want to tweak -> I'll put it back in the generator
For the AsyncLiquibase I'll have a look, that shouldn't be a problem

Then what do you think of this approach? Do you think other classes should go in the lib?

cbornet commented 7 years ago

I think we could provide the classes but not the runtime beans (@Component, @Configuration, etc...). Then we would only generate the beans from the class constructor. What do you think ?

jdubois commented 7 years ago

@cbornet that was my original idea, then the Spring Boot mechanism is very powerful, and I don't really see any downside at the moment

jdubois commented 7 years ago

@gzsombor I just release a new version, with the LoggingAspect back into the generator! For the AsyncLiquibase it's a bit more complex, at the moment it only runs in dev and heroku profiles, otherwise you need to use the "real" Liquibase bean. I'll need to have a closer look.

jdubois commented 7 years ago

A big question I have, and for this I'd like to have all possible (negative) feedback, is what I should do with the JHipsterProperties.

So now I've split the file into 2 files:

JHipsterProperties is in the JHipster lib, and hence is not modifiable. It uses the jhipster key in the application-*.yml files.
ApplicationProperties is generated in the final application. It uses the application key.

At the moment I have just moved the very minimal properties into the JHipster lib, but basically I'm tempted to add all of them:

That's necessary when you do the lib, as you often want to know the current configuration properties
It blocks the configuration, so we have a standard JHipster configuration that every project has, which makes it easier to do third-party tooling - for instance if you work on a module or want to push some configuration from the Registry, at least you are sure those keys exist (otherwise people could modify anything)
It simplifies a lot the final generated code: today we have a huge JHipsterProperties class and I doubt anybody dares touches it
We would still generate the ApplicationProperties file, so the end-user wouldn't lose any power, and would have a clear place to add his own keys

cbornet commented 7 years ago

I also think JHipsterProperties should be immutable. If people want other properties, they can bring their own class.

jdubois commented 7 years ago

OK, I'm doing this as an experiment, we'll see how it goes, and I'll revert the changes if needed.

jdubois commented 7 years ago

I have an issue in the JHipsterProperties with the LoadBalancedResourceDetails, as it's a class with some imports that should be in the classpath. It should be refactored, but I can't find where it's used! It's only available for people using UAA, so I guess only @xetys knows about this -> @xetys do you know if it's being used? At the moment I'm going to suppress it, as it looks unused, but I don't like this...

jdubois commented 7 years ago

OK I found how LoadBalancedResourceDetails but that's just not going to work like that... I need to refactor this @xetys I hope you'll be fine with it

jdubois commented 7 years ago

OK we'll have a new version very soon (release 0.0.4 is syncing with Maven Central), normally @xetys I refactored your code correctly, it should be OK.

I find the end result much clearer for end users, and it's also going to be easier for us to maintain, so at the moment I'm happy with that decision.

Now the real issues are:

How far should we go that way? I'll be very cautious on a first release
How will end-users react to this change? Most people probably won't notice, but who knows...

deepu105 commented 7 years ago

I think what you have moved so far is good for a trial. lets not move too much even if its tempting. lets try to make whats currently moved extensible so that we set a base architecture to mive things easily in the future

Thanks & regards, Deepu

On 14 Dec 2016 1:56 p.m., "Julien Dubois" notifications@github.com wrote:

OK we'll have a new version very soon (release 0.0.4 is syncing with Maven Central), normally @xetys https://github.com/xetys I refactored your code correctly, it should be OK.

I find the end result much clearer for end users, and it's also going to be easier for us to maintain, so at the moment I'm happy with that decision.

Now the real issues are:

How far should we go that way? I'll be very cautious on a first release

How will end-users react to this change? Most people probably won't notice, but who knows...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-267027043, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF7YmLm5H-8fUhD3tyJNYC4FLTDX7ks5rH-eJgaJpZM4LL_Tp .

jdubois commented 7 years ago

Well, there are a few other candidates:

PageableParameterBuilderPlugin(which can be extended by the user if needed) and SwaggerConfiguration (with DEFAULT_INCLUDE_PATTERN which should become a property)
AuditEventConverter
Http401UnauthorizedEntryPoint, SecurityUtils, SpringSecurityAuditorAware and UserNotActivatedException
CachingHttpHeadersFilter (with CACHE_TIME_TO_LIVE which should become a property)

That would definitely clean up the "normal" application, then there are many more classes that should be removed when you select options like MongoDB or OAuth2, but that's less important.

@deepu105 is that OK with you? Is anybody thinking I'm going too far here?

I'm going to ask this on Twitter to have more feedback.

deepu105 commented 7 years ago

These are ok I guess

Thanks & regards, Deepu

On 15 Dec 2016 12:01 a.m., "Julien Dubois" notifications@github.com wrote:

Well, there are a few other candidates:

PageableParameterBuilderPlugin(which can be extended by the user if needed) and SwaggerConfiguration (with DEFAULT_INCLUDE_PATTERN which should become a property)

AuditEventConverter

Http401UnauthorizedEntryPoint, SecurityUtils, SpringSecurityAuditorAware and UserNotActivatedException

CachingHttpHeadersFilter (with CACHE_TIME_TO_LIVE which should become a property)

That would definitely clean up the "normal" application, then there are many more classes that should be removed when you select options like MongoDB or OAuth2, but that's less important.

@deepu105 https://github.com/deepu105 is that OK with you? Is anybody thinking I'm going too far here?

I'm going to ask this on Twitter to have more feedback.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-267184346, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlFzRY-pfmXgqg2eYDM7eDUz86MrYyks5rIHVTgaJpZM4LL_Tp .

xetys commented 7 years ago

I will have to test this ASAP...As I am using it every day so it won't take me too long on that.

I just need to survive my oral exam tomorrow, to be a free man, again :tired_face:

jdubois commented 7 years ago

I couldn't migrate all of them, because some of them had links to the generated code. Once again, I'm being very cautious here. I also made them all configurable, so they should be easy to override or modify if needed.

Here's my latest commit: https://github.com/jhipster/generator-jhipster/commit/a820888cf14e7bc69d36d9add72b95a00e186ef5 and this is linked to release 0.0.5 of the library (that is currently syncing with Maven Central, so my build will fail, I know...).

As far as I'm concerned it's finished for the "normal" option, I could also have a look at some options like Cassandra or Mongodb, but this is less important.

deepu105 commented 7 years ago

Something worth moving are the stuff we do for cassandra like changelog management repository etc

Thanks & Regards, Deepu

On Thu, Dec 15, 2016 at 6:56 PM, Julien Dubois notifications@github.com wrote:

I couldn't migrate all of them, because some of them had links to the generated code. Once again, I'm being very cautious here. I also made them all configurable, so they should be easy to override or modify if needed.

Here's my latest commit: a820888 https://github.com/jhipster/generator-jhipster/commit/a820888cf14e7bc69d36d9add72b95a00e186ef5 and this is linked to release 0.0.5 of the library (that is currently syncing with Maven Central, so my build will fail, I know...).

As far as I'm concerned it's finished for the "normal" option, I could also have a look at some options like Cassandra or Mongodb, but this is less important.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-267395745, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF8vN6hs66NuotSW9ILWR3WivumDlks5rIX9CgaJpZM4LL_Tp .

jdubois commented 7 years ago

Sorry I closed this by mistake.

So here is the current state:

I'm doing one last (0.0.6) release, and for me the library should be OK
The vote on Twitter is quite clear, for using the library, then there is an important share of people which are unhappy.
The more I use it, the better I find it. It definitely cleans up the generated application from a lot of hacks/tuning that people will call "verbose". I'm quite convinced it's a better option.
My main issue is that JHipster is transforming from a simple generator to a real framework. But I think that was bound to happen, given everything we do.

I hope everybody is happy if I merge this soon! Please comment or react with emoticons!

deepu105 commented 7 years ago

I would like people to call it a framework :smile:

jdubois commented 7 years ago

Closing as the PR is merged!

deepu105 commented 7 years ago

@jhipster/developers since we are moving backend stuff to a lib why not do the same for frontend libs as well? we have the perfect candidate for that (our translate loader, alert directive and some util services) I already have a package called jhipster in NPM and we can use that what do you think?

deepu105 commented 7 years ago

It will greatly simplify our ng2 shared directory (the above mentioned are stuff people would rarely change)

jdubois commented 7 years ago

Yes @deepu105 that was our original plan. Now that means releases will be even more complex for us :-( Can you move the "jhipster" project to the "jhipster" org? And yes I need to give you access to it.

jdubois commented 7 years ago

Oh sorry, I have the "jhipster" org, and "generator-jhipster" is still under my name, not in the org! I'm testing the migration

jdubois commented 7 years ago

Oh now I remember why we don't have an org, it costs $14 per month! That's really not nice... And then I'm pretty sure we will need to have paying accounts, etc... I'll send an email on the dev mailing list so we can discuss this.

deepu105 commented 7 years ago

I guess we dont have to tie releases together. We can release maven library and npm library if there is any update done and add it to generator on next release so practically it will like using any other third party lib.

I assume you are talking about NPM orgs. I guess we dont need to have orgs we can add colloborators to the lib. Ill add you as collaborator to the jhipster lib. On the github repo can you create a new repo called 'ng-jhipster' and give me access to that?

Thanks & regards, Deepu

On 25 Dec 2016 3:15 p.m., "Julien Dubois" notifications@github.com wrote:

Oh now I remember why we don't have an org, it costs $14 per month! That's really not nice... And then I'm pretty sure we will need to have paying accounts, etc... I'll send an email on the dev mailing list so we can discuss this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-269124443, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF2-Vi4Ivu3ws3k9XLMXHgsZn-0cpks5rLnp5gaJpZM4LL_Tp .

jdubois commented 7 years ago

Yes but I'd like to have the same release numbers for the generator, the server and the client lib, so it's easier for people to understand. So basically they are tied.

jdubois commented 7 years ago

@deepu105 I've added you as a "collaborator" to "generator-jhipster", can you do the same with me for "jhipster"? Then at some point we might need to use the "jhipster" user, and give him full access to everything

deepu105 commented 7 years ago

@jdubois that would be difficult. Imagine we have a bugfix in just the generator then we would be needing to release the maven lib and angular lib un necessarily. I would rather keep separation of concerns and treat them like any other lib (like jhipster-core) we use so that they can have their individual release cycles and I'm sure that people are smart enough to know

deepu105 commented 7 years ago

Seems I'm not admin in the github org so can you please help to create a repo called ng-jhipster under our jhipster github org and make me an admin for it?

deepu105 commented 7 years ago

I have added you as collaborator for the jhipster npm namespace. How would you like the npm library to be called ng-jhipster or jhipster ? if we do ng-jhipster it will give us the freedom to retain jhipster namespace in the future say if we wanna break free from yeoman and be a standalone generator like angular-cli then we could publish it as jhipster at that time.

jdubois commented 7 years ago

Oh the ng-jhipster is already created when you first asked for it :-) All members of the dev team have write access to it, like for generator-jhipster

deepu105 commented 7 years ago

nice :)

Thanks & Regards, Deepu

On Sun, Dec 25, 2016 at 5:34 PM, Julien Dubois notifications@github.com wrote:

Oh the ng-jhipster is already created when you first asked for it :-) All members of the dev team have write access to it, like for generator-jhipster

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-269129252, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF31l5zSjmqrqrieqF0bVnG980doVks5rLpsdgaJpZM4LL_Tp .

jdubois commented 7 years ago

Yes let's call it ng-jhipster, as we might also use something else than Angular at some point

deepu105 commented 7 years ago

ok ill move something to it soon.

jdubois commented 7 years ago

OK, let's do it this way, it's simple and doesn't have any specific impact on what we usually do. I would just do a reference to the JHipster libs when we do the release notes for a new generator version.

deepu105 commented 7 years ago

is that will be perfect and betetr for maintanance and people can always refer to the POM/package json to see what version is used

Thanks & Regards, Deepu

On Mon, Dec 26, 2016 at 11:45 AM, Julien Dubois notifications@github.com wrote:

OK, let's do it this way, it's simple and doesn't have any specific impact on what we usually do. I would just do a reference to the JHipster libs when we do the release notes for a new generator version.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-269198971, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlF1VaoHqX1MQKblaaqYNpqeECBQv-ks5rL5rNgaJpZM4LL_Tp .

gmarziou commented 7 years ago

It may require some instructions for contributors, all of them surely know how to make it work for the java lib either by importing its module in IDE or using .m2 but personally I have no idea how to make the same for an npm library

deepu105 commented 7 years ago

With typescript its similar in an IDE

Thanks & regards, Deepu

On 26 Dec 2016 4:17 p.m., "Gaël Marziou" notifications@github.com wrote:

Think to contributors, all of them surely knows how to make it work for the lib either by importing its module in IDE or using .m2 but personally I have no idea how to make it the same for a npm library

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/jhipster/generator-jhipster/issues/4674#issuecomment-269219548, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDlFyy6FoN91Xhni7vLaRhnsq55FTJ_ks5rL9qcgaJpZM4LL_Tp .

jhipster / generator-jhipster

Review the JHipster library #4674