apollographql / apollo-kotlin

:rocket: Β A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.75k stars 651 forks source link

Publish a BOM to align all versions #3728

Open martinbonnin opened 2 years ago

martinbonnin commented 2 years ago

Publish a BOM so that users can import it and enforce the same version everywhere.

martinbonnin commented 7 months ago

@jsmestad, @lwasyl, @mateuszkwiecinski do you have any specifics about where this would be needed? I can't remember why I opened this πŸ˜… and as far as I can tell, as long as we observe semantic versioning (i.e. no breaking changes in non-major versions), the libraries should play nicely together. What case am I missing here?

mateuszkwiecinski commented 6 months ago

do you have any specifics about where this would be needed?

I personally can't say anything beyond typical arguments for using Platforms, that is to ensure resolving desired version of dependencies between various configuratons/subprojects, including dependencies resolved transitively. While you already provide cross-configuration correctness between plugin classpath and library dependencies (or at least that's what I understood checkApolloVersions is for), it's not guaranteed the same versions are resolved between Gradle modules and that's where a BOM applied as enforcedPlatform dependency would be handy. You do a great job in ensuring backwards compatibility, so it doesn't sound like a significant issue and it's unlikely any project would fail in runtime due to changes in public api, but for the sake of correctnes, taking into account the number of modules you publish - a BOM doesn't sound lika bad idea.

Thinking of some examples where a BOM could be helpful, I can imagine a setup that looks as follows:

-> app 
-->libA - explicitly declares dependency on apollo-kotlin 4.1
-->libB - transitively resolves dependency on apollo-kotlin 3.8
-->libC - due to misconfiguration declares dependency on apollo-kotlin 4.0

and this could be very much hard to debug. Running anything from app will resolve the highest version, but running i.e. tests in libC will use older version of the dependency. If there was a regression in 4.1, one wouldn't be able to reproduce it in neither tests of libB nor libC. Things get much trickier with transitive dependencies, which are usually obscure and more difficult to identify and track.

I can also share setup of my $workProject: it publishes its own platform which define contstraints on libraries we care about and apply it as a enforcedPlatform to all configurations/modules. That way, regardless of the module we run tasks from - it should resolve in most cases consistently with the "main" project. The disadvantage is, for apollo-kotlin, we only list dependencies we are aware of - so if you introduced a new module and it somehow leaked to our dependency graph, it then would resolve to the default version, not the one we explicitly would try to request. If there was a apollo-kotlin-bom artifact we would be sure all apollo-kotlin modules use the requested version.

tl;dr it seems like it's mostly about ensuring proper version of dependencies resolved transitively (that's my conclusion after writing this comment πŸ˜…)

martinbonnin commented 6 months ago

Thanks for all the details!

a BOM applied as enforcedPlatform dependency would be handy

TIL about enforcedPlatform, thanks! From the docs:

An enforced platform is a platform for which the direct dependencies are forced, 
meaning that they would override any other version found in the graph.

If I understand this right, this is quite dangerous though. If a transitive dependency requires a new symbol available only in apollo-runtime:4.2 and your app is still using apollo-bom:4.0 then app crashes at runtime? Doesn't feel great.

If there was a regression in 4.1, one wouldn't be able to reproduce it in neither tests of libB nor libC.

Right. If your app ends up pulling a newer version of a transitive dependency, your lib unit tests are not run with the same classpath as your app.

The disadvantage is, for apollo-kotlin, we only list dependencies we are aware of.

Gotcha πŸ‘

But also... Playing the devil's advocate here, this is true of any dependency. If okhttp starts depending on a new artifact, let's say kotlinx-io, your dependency graph now has an "uncontrolled" dependency again (kotlinx-io). Unless okhttp puts kotlinx-io in its BOM but I don't think it's how BOMs are supposed to be working?

The problem is only more visible in apollo-kotlin because we have lots of modules but I don't think BOM is the proper solution there?

Now for what the proper solution, to be honest, I'm not really sure πŸ˜… . Looks like what you want is something that dumps your app's resolved runtimeClasspath to a file that can be later used in the lib tests (and fails if by any chance the app version is < the default lib version).

Does that make any sense?

mateuszkwiecinski commented 6 months ago

If a transitive dependency requires a new symbol available only in apollo-runtime:4.2 and your app is still using apollo-bom:4.0 then app crashes at runtime?

That's correct :/ I'd try to justify ourselves by saying we trust our test suite (so we don't afraid about runtime crashes) and it looks like the team values build reproducibility/consciousness of what dependencies are pulled in. If we wanted to take a less radical approach we could just switch enforcedPlatform to platfrom and let dependencies to be upgraded during dependency resolution.

your dependency graph now has an "uncontrolled" dependency again (kotlinx-io). Unless okhttp puts kotlinx-io in its BOM but I don't think it's how BOMs are supposed to be working?

Again, fair point πŸ‘ If the new dependency isn't already tracked by our platform or one of platforms we depend on then it would result in an "uncontrolled" dependency. I can try reasoning it by saying if the new dependency proves to be problematic or crucial to the app we could probably start controlling its version, but in most cases I'd expect the new dependency to be unique to the pulling module. (exmaple: for apollo-kotlin dependencies, we control version of apollo-kotlin itself, but also okhttp (to ensure its version is resolved consistently between modules), but we don't enforce version of com.benasher44:uuid as an example of transitive dependency unique to the pulling module). For more context I can also share the way we somewhat monitor changes in transitive dependencies is by using Jake Wharton's dependency-tree-diff tool, so we get a preview of a dependency graph changes on corresponding pull request + we apply a pinch of common senseβ„’ to decide on how to proceed.

Now for what the proper solution, to be honest, I'm not really sure πŸ˜…

I personally don't feel there is a single proper solution that suits all cases πŸ˜… It feels like people may have preferences on how they want to declare/control their dependencies. Some may prefer sharing version via version catalogs, others may want to use third-party plugins (like refreshVersions), some could prefer full-on dependency locking or having custom dependencyResolution strategy, leveragin BOMs could be one of those preferences.

With the motivation to give user an option, how big of an effort would be it introduce a BOM? What are the disadvantages? It doesn't feel too complex in terms of the setup, examples: ktlint, retrofit For more context I can add there are many popular projects that publish their BOMs: kotlin, coroutines, compose, junit, okhttp + already mentioned ktlint and retrofit

(+ note at the end, just to be clear on my intention here: I'm not arguing why you should publish a BOM, but rather try to explain my PoV on why I find BOMs useful. so if you don't feel like it fits apollo-kotlin needs, I personally won't mind closing the issue for now - this is not a blocker in any way for me/us)

martinbonnin commented 6 months ago

Thanks for all that discussion! I definitely don't want to discard the idea of a BOM if that can help and your concerns do make a ton of sense πŸ‘

My main worry is almost conceptual at this point. I wouldn't want to introduce something that solves the the problem "wrong" and have to advocate another solution 2 years from now.

But I do agree that the practical aspects matter too and that "Imperfect solution now" > "Perfect solution maybe later". It might be relatively easy to publish a BOM. I'll dig a bit more and keep you posted!

martinbonnin commented 6 months ago

For the record, link the OkHttp BOM PR