crossbario / autobahn-java

WebSocket & WAMP in Java for Android and Java 8
https://crossbar.io/autobahn
MIT License
1.52k stars 425 forks source link

Split Autobahn into Gradle modules by feature #342

Closed prefanatic closed 6 years ago

prefanatic commented 6 years ago

Why Leveraging a multi-module project will allow consumers of Autobahn to select what features of Autobahn they need (see transport options). Multi-module also allows easier scalability with the cleaner code separation, as opposed to separating in one module via 'sourceSets'

What's been done:

Current Problems:

Other Notes:

tl;dr I decoupled a bunch of autobahn-java components into modules.

I'd like to be able to work with you (the maintainer[s]) on this - this is a significant change, and I understand that there will be quite a bit back on this in order to find the right implementation.

PR can be found here: #343

om26er commented 6 years ago

Thanks for contributing, before I do a detailed review of this and get @oberstet opinion, I think it would be really great to split TicketAuth implementation out of this.

Retrolambda has to be used to allow 'autobahn-android' to work on API < 26. We may be able to leverage desugar in AGP3.0, but I can't seem to get that functional for android-library modules.

Android Studio 3.0 introduced native lambdas support that works on older versions of Android as well, so we won't need retrolambda.

prefanatic commented 6 years ago

@om26er

Sounds good. The ticket auth has been removed from this PR - so this now only references the split into modules. I'll continue to work on the ticket auth in parallel in a nicer way, and setup a PR for it sometime in the future.

Android Studio 3.0 introduced native lambdas support that works on older versions of Android as well, so we won't need retrolambda.

Unfortunately, this will only work for the 'autobahn-android' module, and not the 'autobahn-java' with all the actual WAMP protocol stuff. The desugaring of the lambdas comes as a feature in AGP3.0, which we should only apply to the Android module.

After looking at 'autobahn-android' - it seems that it makes more sense to pull this apart even further and create 'autobanh-transports-androidwebsocket'. In this scenario, the dynamic application of AGP3.0 can be reapplied back to 'autobahn'.

However again, ideally, CFuture usage within the library should be removed and exposed as an adapter to consumers. In this case, lambdas might as well be removed and desugaring/retrolambda wouldn't be required.

oberstet commented 6 years ago

Restructuring the whole library into modules: my view is: I don't feel the need for it and it is a very invasive change. It would throw us back, when we have much more important gaps to close in ABJ. Ticket authentication is definitely one. In fact, all the WAMP authentication methods. Also, Kotlin: ABJ itself should be pure Java (otherwise we end up with a split language codebase). Then, OkHttp transport: there is no HTTP2 transport binding for WAMP specified.

prefanatic commented 6 years ago

Restructuring the whole library into modules: my view is: I don't feel the need for it and it is a very invasive change. It would throw us back, when we have much more important gaps to close in ABJ.

The PR may be a little overboard in terms of what it is trying to bring in, but looking at the basic premise of splitting out the two transports (netty & android websocket) from the actual implementation of the protocol is not invasive at the API and implementation level. It's only visibly invasive in the build script. Consumers of the library would only need to specify and additional dependency for the transport they wish to use. The code doesn't change at all, and is completely transparent beyond the build.

If the concern is that consumers have to change their build script to depend on an additional transport, even this can be transparent - a module can be provided that exposes ABJ in the same way it is now, coupled with the regular 'autobahn-java' and additionally 'autobahn-transports-netty/androidwebsocket'.

Also, Kotlin: ABJ itself should be pure Java (otherwise we end up with a split language codebase). Then, OkHttp transport: there is no HTTP2 transport binding for WAMP specified.

Makes sense. I'll maintain an OkHttp transport myself elsewhere. OkHttp is only being used for it's websocket implementation - it isn't only HTTP2, so I'm unsure what you mean by the latter part of your comment.

oberstet commented 6 years ago

OkHttp is only being used for it's websocket implementation

Ah, I see - then my comment is void.

looking at the basic premise of splitting out the two transports (netty & android websocket) from the actual implementation of the protocol

My personal view on this is actually 2 fold:

I want to write code that does stuff, not fiddle with complexities of the build system that bring no value.

This is my personal view, and maybe I am wrong, and there is a need.

I would suggest leaving this issue open for now to collect more user feedback first. If that feedback shows interest in what you propose, I'll bite the bullet;)

oberstet commented 6 years ago

Closing, as this isn't a priority for now fo us, and no interest has shown up ..