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

added weld / CDI #72

Closed
wants to merge 3 commits into from
Closed

added weld / CDI #72

wants to merge 3 commits into from

Conversation

RainerW
Copy link
Contributor

@RainerW RainerW commented Jan 5, 2014

Added JBoss weld (http://weld.cdi-spec.org/) for CDI support

why:

  • no static var problem in tests
  • code gets testable
  • future code can now use CDI to decouple stuff ( e.g. melix modularization code could use this )

did:

@rajmahendra
Copy link

OMG there is no like to click +1

@RainerW
Copy link
Contributor Author

RainerW commented Jan 5, 2014

@rajmahendra :) thx. But did oversee a minor runtime issue, now fixed.

@melix
Copy link
Contributor

melix commented Jan 6, 2014

I'm not sure adding a dependency on JBoss Weld is a good idea. If you really want to use DI (and honestly not sure it's worth it here), can't you just use Google Guice?

@RainerW
Copy link
Contributor Author

RainerW commented Jan 6, 2014

Where is the difference between a dependency to google or JBoss? They are both just companies? ( If weld is not your choice, i would rather take Spring, because in the end this is much more powerfull in the long run)

() : DI makes the existing tests controllable. Not having static references in your code does decouple your logic and replacing parts make it much easier.

@melix
Copy link
Contributor

melix commented Jan 6, 2014

CDI requires an actual implementation, which makes it harder to embed. Now, the tests shouldn't even need static methods, CDI or not. CDI makes decoupling easier, but it's not CDI that allows decoupling. It's all about how the code is designed.

Now, I'm thinking about embedded JBake (I'm targetting Gradle, but I know other people looking into integration into an application). If you use Guice, CDI (Weld or anything else) or Spring, it makes things harder because you might already depend on one of those. And you don't want to include two dependency injection frameworks.

@vietj
Copy link

vietj commented Jan 6, 2014

I also think that using a DI framework will make the framework hard to reuse outside in other runtimes than jbake itself, so my opinion is to avoid DI framework and just use simple Java instead.

@RainerW
Copy link
Contributor Author

RainerW commented Jan 6, 2014

Ok then i think i misunderstoot jbake. i was not aware thie is an library. I viewd it as an application. But then probably jbake should be splitted, and yes DI inside an library is normally not necessary, except you want to provide an lightweight Plugin mechanism on top of it.

In that library scenario, having an static ConfigUtil instance is really bad!

So then JBake needs to get splitted into a jbake-core and an jbake-main. jbake-main could then still use DI, where jbake-core would be a classic Library without DI?

@jonbullock
Copy link
Member

@RainerW thanks very much for the contribution, much appreciated. As I originally said on #9 I didn't see any major need to add CDI support at this time and as others have mentioned there are some downsides when CDI support is included in terms of integration. Today JBake is an application and used as such by quite a few users, however others have expressed desire to include it as a library or framework in other applications and right now I don't want to shut the door on those use cases. I'd like to see the user base grow and these other uses will definitely help that goal.

At some point soon I'll move the repo to the organisation, once the re-architecture PR's are merged in. Then we can look at potentially splitting what JBake is today into smaller parts if applicable.

For the time being I'll leave this open until the above is done and I can review.

Thanks again.

@rajmahendra
Copy link

@jonbullock @RainerW If we plan to support this issue then i feel #18 will get a good platform. I feel we can create JBake Plugins using CDI Extension! Just a wild mindmap view need to thing properly on this.

May be we can look into both in v3.0

@jonbullock
Copy link
Member

@rajmahendra Definitely food for thought, I can see v3.0 being a big release :-)

@aalmiray
Copy link
Contributor

aalmiray commented Feb 2, 2014

If DI is still on the table I'd suggest sticking to JSR-330 and use Guice/Guiceberry for testing. I've nothing against JBoss/Weld but clearly CDI is much more that just plain DI. @vietj can attest to the benefits/troubles of supporting Guice, Spring and Weld under a common umbrella (see https://github.com/juzu/juzu).

@vietj
Copy link

vietj commented Feb 3, 2014

Juzu is able to run with those three containers indeed. The cost is to have an abstraction and have good tests to ensure same behavior (https://github.com/juzu/juzu/tree/master/core/src/main/java/juzu/impl/inject) .The benefit is to leave the user the choice of the DI which in case of a web framework is very important. Somehow I should extract the Juzu code outside of Juzu as it is not coupled to the framework itself.

@jonbullock
Copy link
Member

Thanks for the advice everyone.

@RainerW
Copy link
Contributor Author

RainerW commented Feb 18, 2014

I suggest splitting JBake into an core an and CLI tooling. Where core should not have any DI (At least visible to the outside world) But the CLI Tooling could use an DI. Also my own app which just uses JBake-core would use their DI implementation.
I don't think DI should be in JBake-core. And DI should not be used for an plugin infrastructure, they have completely different requirements, plugin need an livecycle to dynamically stop/start update then. Use OSGi for that.

@lefou lefou mentioned this pull request Feb 19, 2014
@jonbullock
Copy link
Member

I'm going to close this off now. The code base has changed significantly since this PR was proposed so it's not going to be merged as it is anyway.

Thanks to everyone for participating in this discussion and providing valuable opinions, especially to @RainerW for the initial PR.

@jonbullock jonbullock closed this May 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants