java-client icon indicating copy to clipboard operation
java-client copied to clipboard

Shaded and non shaded versions

Open danieldbower opened this issue 2 years ago • 9 comments

Corrects the url of the sdk documentation as the given url does not work.

Minor fix added to specify the version number for the maven-source-plugin

I also bumped the version to ensure I had some maven settings correct.

I was not able to run the redis module tests as that seems to be an integration test requiring some infrastructure.

The pom will now create a shaded version java-client-with-deps and a non-shaded version at java-client.

In this version, documentation referencing the shaded version's maven coordinates would have to change from this:

            <dependency>
                <groupId>io.split.client</groupId>
                <artifactId>java-client</artifactId>
                <version>4.9.0</version>
            </dependency>

to this

            <dependency>
                <groupId>io.split.client</groupId>
                <artifactId>java-client-with-deps</artifactId>
                <version>4.9.0</version>
            </dependency>

The original maven coordinates would be for the non-shaded version.

Why do this? Simpler management of dependencies with tools like scanning for vulnerabilities and updates.

danieldbower avatar Aug 03 '23 15:08 danieldbower

While this works, I'm increasingly convinced that the correct way to do it is a separate module so that the dependency list in the pom is correct in both instances.

danieldbower avatar Aug 03 '23 15:08 danieldbower

Reasoning for the separate module: https://www.mail-archive.com/[email protected]/msg139854.html

danieldbower avatar Aug 03 '23 16:08 danieldbower

I'm using this package in a kotlin project. While it's working fine using JVM, it fails when using the native build (Graalvm). We believe the native build is not able to load some of the shaded dependencies. It's quite hard to prove since SplitClientImp says the sdk is ready but all calls to getTreatment are failling. There are no other warnings or errors. It seems to at least call the API since we're getting a 400 Bad request error when using an invalid API Key.

jbreton avatar Nov 07 '23 16:11 jbreton

Are there any suggestions or concerns with this PR that would increase its chances of being merged?

danieldbower avatar Apr 30 '24 18:04 danieldbower

If you REALLY prefer to shade, then you should be prefixing those packages so they don't conflict with the rest of the app. See this feature of the maven-shade-plugin: https://maven.apache.org/plugins/maven-shade-plugin/examples/class-relocation.html

danieldbower avatar Apr 30 '24 20:04 danieldbower

Hi @danieldbower , We apologize for this not having gained traction yet, and thank you for the valuable contribution. Our team is currently analyzing this as it would mean supporting two packages along with the associated maintenance overhead. We expect to have a resolution for this in the following quarters.

agustinona avatar Sep 05 '24 21:09 agustinona

I'd recommend that if you wanted to stick with just one artifact, you go with the non-shaded version. That will remove some complexity and make the library more secure. If you want to stay with the shaded version, please prefix those packages so they don't conflict with the rest of the app. See this feature of the maven-shade-plugin: https://maven.apache.org/plugins/maven-shade-plugin/examples/class-relocation.html

danieldbower avatar Sep 09 '24 15:09 danieldbower