AlessioDP / libby

A runtime dependency management library for plugins running in Java-based Minecraft server platforms.
MIT License
81 stars 20 forks source link

Download nanojson at runtime #30

Closed frengor closed 1 year ago

frengor commented 1 year ago

This PR changes nanojson to be downloaded at runtime. This way users who doesn't use the JSON configuration doesn't need to have it shaded. This also replaces every call of .replace("{}", ".") with a call to Util#replaceWithDots(String).

Includes and excludes properties of relocations are now configurable in the JSON configuration (it should now have feature parity with the Library's builder). The methods equals and hashCode have also been implemented for Relocation to allow testing the includes and excludes properties.

Lastly, as an implementation detail, now a Set is used for fetched objects which override equals and hashCode to avoid duplicates.

AlessioDP commented 1 year ago

NanoJSON shading was discussed here: https://github.com/AlessioDP/libby/pull/23

Its so small that it can be shaded. The same concept should be used for jar-relocator too, but it needs more transitive dependencies.

I was thinking to handle libby-core:jar as no shaded library, on runtime download dependencies, and make a different classifier for core with shaded libraries, like libby-core:full.

What do you think about it?

frengor commented 1 year ago

Its so small that it can be shaded. The same concept should be used for jar-relocator too, but it needs more transitive dependencies.

Yes, but what if the JSON configuration doesn't get used? Downloading it at runtime means that you don't pay the 25kb of nanojson if you don't use it. Yes, it is small, but the entire concept of Libby is to download at runtime, so I think the change actually makes sense.
I think the major argument against this is the amount of reflection needed, to which I agree completely (the fact that JsonObject extends LinkedHashMap and JsonArray extends ArrayList improves the situation here, but it's still suboptimal).

Maybe doing something similar to TransitiveDependencyCollector could be the way here? (this could have circular dependencies issues though)

Another idea could also be creating another module which shades nanojson and moving the configuration stuff there (this would imply moving the LibraryManager#configureFromJSON(...) methods into another class).

I was thinking to handle libby-core:jar as no shaded library, on runtime download dependencies, and make a different classifier for core with shaded libraries, like libby-core:full.

What do you think about it?

Reading this a second time, I don't fully understand what you're proposing here: would libby-core:jar have the same features as libby-core:full? (in which case, having two classifiers doesn't really make sense, since every feature would be already implemented with runtime downloading of dependencies)

AlessioDP commented 1 year ago

With shadow plugin, we are shading and relocating NanoJSON into the output normal jar. Its possible to set a classifier, as example "full", to separate the output and use it like this:

// Normal implementation
implementation("com.alessiodp.libby:libby-core:2.0.0-SNAPSHOT")

// With shade
implementation("com.alessiodp.libby:libby-core:2.0.0-SNAPSHOT:full")
frengor commented 1 year ago

With shadow plugin, we are shading and relocating NanoJSON into the output normal jar. Its possible to set a classifier, as example "full", to separate the output and use it like this:

// Normal implementation
implementation("com.alessiodp.libby:libby-core:2.0.0-SNAPSHOT")

// With shade
implementation("com.alessiodp.libby:libby-core:2.0.0-SNAPSHOT:full")

So, if I understand correctly, the normal implementation would download nanojson at runtime, while the full one will shade it?

AlessioDP commented 1 year ago

Exactly, it is possible to check if NanoJSON is available runtime (Class checks) and use it otherwise just download it like jar-relocator.

frengor commented 1 year ago

otherwise just download it like jar-relocator.

jar-relocator is loaded into an IsolatedClassLoader, will nanojson be also loaded into an IsolatedClassLoader?