Skip to content

Conversation

@leeeyubin
Copy link

@leeeyubin leeeyubin commented Jun 2, 2025

Issue

Overview (Required)

  • 기존 Retrofit을 사용하던 부분을 Ktor로 마이그레이션 하였습니다.
  • Ktor에 대한 의존성이 SessionApi가 있는 모듈로 전이되지 않도록, DroidKnightsNetwork라는 추상화 계층을 도입하고 get 함수를 정의해 분리해두었습니다
    • 이때 HttpResponse.body()의 동작 방식인 typeInfo를 활용하였습니다.
    • 각 플랫폼 별로 맞는 HttpClientEngine을 사용하였습니다.
  • coreNetworkModule 모듈을 생성하여 network 코드 관련 의존성을 주입해 주었습니다.

Screenshot

@leeeyubin leeeyubin force-pushed the feature/#409-ktor branch from 6497f63 to ad562ec Compare June 4, 2025 16:47
@leeeyubin leeeyubin marked this pull request as ready for review June 4, 2025 17:10
@leeeyubin
Copy link
Author

leeeyubin commented Jun 4, 2025

코드리뷰 달아주신 내용을 토대로 수정하여 PR 올렸습니다! 확인부탁드려요:)

Copy link
Member

@taehwandev taehwandev left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.

@leeeyubin leeeyubin force-pushed the feature/#409-ktor branch 2 times, most recently from 976db7f to da8898b Compare June 5, 2025 12:56
@leeeyubin
Copy link
Author

안녕하세요!
Koin Verify Test가 추가됨에 따라 HttpClientEngine이 플랫폼별로 설정되지 않아 CI 오류가 발생했었습니다.
이에 플랫폼별로 적절한 HttpClientEngine을 설정하여 문제를 해결하였습니다! dcb03e0
확인해주시면 감사하겠습니다. 🙇‍♀️

leeeyubin added 23 commits June 6, 2025 23:06
@leeeyubin leeeyubin force-pushed the feature/#409-ktor branch from 3830e51 to 930e05b Compare June 6, 2025 14:06
Copy link
Contributor

@workspace workspace left a comment

Choose a reason for hiding this comment

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

@leeeyubin 긴 티키타카 속에 approve를 드리게 되어 기쁩니다 ㅎㅎ CI 터지는건 제가 좀 살펴볼게요! 쉽게 고칠 수 있는거면 추가로 comment 남겨드리고, 아니면 머지 후 고치도록 하겠습니다.

수고하셨습니다 👍 💯

@workspace
Copy link
Contributor

workspace commented Jun 6, 2025

@leeeyubin InsertKoinIO/koin#2029

이 이슈랑 같은 이슈로 보이는데요,

  1. composeApp module의 desktopTest sourceSet에 ktor-client-core 의존성을 추가하고
  2. 위 링크의 마지막 댓글과 같이 verify 함수에 extraTypes = listOf(HttpClientEngine::class, HttpClientConfig::class)을 기입하여 문제를 해결합시다! (성공하는 것 확인)

verify함수에 왜 extraTypes를 넣게 되었는지 추적하기위해 간단한 설명과 함께 위 링크를 주석으로 추가해주세요.

사실 요 pr 뒤로 할 작업들이 좀 밀려있어서, 제가 퀵하게 수정해보겠습니다.

수고하셨습니다!!

@workspace workspace merged commit 397c8b8 into droidknights:2025/compose-multiplatform Jun 6, 2025
8 of 9 checks passed
@workspace
Copy link
Contributor

@leeeyubin #504

위 pr에서

  • httpClient를 di를 통해 주입 받는 것이 아니라 DroidKnightsNetwork 내에서 초기화 하도록 수정
    • 굳이 HttpClient를 di에 노출할 필요가 있을까 생각이 들었습니다.
  • HttpClientEngine.kt -> HttpClient.kt로 rename 및 package 이동

체크 다 돌면 일단 머지할 예정인데요, 다른 의견이 있으시다면 pr에 댓글 남겨주세요 ㅎㅎ

@leeeyubin
Copy link
Author

넵! 바뀐 부분 PR로 확인했습니다!
DroidKnightsNetwork 내부에서 초기화하는 방식이 확실히 깔끔하고 좋은 방향인 것 같아요.
부족한 점도 있었겠지만, 이번 Ktor 마이그레이션에 기여하면서 많이 배우고 큰 도움 받았습니다. 감사합니다! @workspace 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants