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

Update KMP Compatibility Guide for 1.9.20 #3789

Merged
merged 11 commits into from
Oct 31, 2023

Conversation

dsavvinov
Copy link
Contributor

No description provided.

```

While this approach requires more initial setup work, it doesn't use any low-level entities of Gradle and KGP, making
easier to work with the resulting build and maintain it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think where this falls short is tests? Suppose you had a common test suite that tested both ktor and okhttp clients. The code in the tests is all the same, it just expects a HTTP client:

// commonMain
interface HttpClient { ... }
expect fun HttpClient(): HttpClient

// commonTest
@Test 
fun testClient() {
  assertThat(HttpClient().get("https://...").data).isEqualsTo("Hello World")
}

With multiple JVM targets, it was only a matter of defining 2 actuals:

// ktorMain
actual fun HttpClient() = KtorHttpClient()

// okHttpMain
actual fun HttpClient() = OkHttpClient()

Now with different Gradle projects, how would one do this? expect/actual do not cross Gradle projects boundaries. And duplicating the test code doesn't feel great. Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. The brutally honest description for this deprecation would've been something like: "if you've encountered this deprecation, and you don't see an easy way to migrate your project, then reach us and tell more about your case".

But that would be a bit unfriendly, so we tried our best to provide some intrsuctions which applicable at least in some cases. It is expected that these instructions miss some cases, and we'll be happy to learn if someone encounters such cases in real projects

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have this exact expect/actual scenario in this integration test. I've turned this problem upside down but still not sure how to convert. Any guidance would be super welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took another look at it. In the current state, it's working with a single JVM target and 2 compilations but shows this warning:

w: Kotlin Source Set 'ktorTest' can't depend on 'commonTest' as they are from different Source Set Trees.
Please remove this dependency edge.

I'm not sure how to remove the common source set without breaking the expect/actual

@danil-pavlov danil-pavlov force-pushed the dsavvinov/compatiblity-guide-1.9.20 branch from 563b884 to c808e7c Compare October 17, 2023 17:06
@danil-pavlov danil-pavlov changed the base branch from master to 1-9-20-docs October 17, 2023 17:07
@danil-pavlov danil-pavlov force-pushed the dsavvinov/compatiblity-guide-1.9.20 branch from c808e7c to e17d346 Compare October 17, 2023 17:15
Copy link
Contributor Author

@dsavvinov dsavvinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a loot at my comment about forms of verbs in deprecation timeline sections and about "similat target types". Everything else is a minor

@danil-pavlov danil-pavlov force-pushed the dsavvinov/compatiblity-guide-1.9.20 branch from f1c0961 to f47f597 Compare October 30, 2023 18:27
@danil-pavlov danil-pavlov merged commit e13aeef into 1-9-20-docs Oct 31, 2023
@danil-pavlov danil-pavlov deleted the dsavvinov/compatiblity-guide-1.9.20 branch October 31, 2023 11:55
@danil-pavlov danil-pavlov mentioned this pull request Nov 16, 2023
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.

4 participants