-
Notifications
You must be signed in to change notification settings - Fork 5
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
Moya 및 Combine을 사용한 Network 리펙토링 #163
base: master
Are you sure you want to change the base?
Conversation
getAppVersionUseCase.execute() | ||
.sink { completion in | ||
if case .failure(let networkError) = completion { | ||
print("ERROR", #function, networkError) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(질문) 버전 불러오지 못했을 때 앱에 접근 가능하도록 하는것 같은데,
강제 업뎃이 필요한 상황과 맞지 않는것 같아서 이유가 궁금하네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24.10.28 (iOS 회의 결과)
- print문 외에 사용자에게 노출되는 팝업 추가하기 (ex. 버전 불러오기에 실패 했다)
static func parameters(from encodable: Encodable) -> [String: Any] { | ||
let encoder = JSONEncoder() | ||
guard let data = try? encoder.encode(encodable), | ||
let dictionary = try? JSONSerialization.jsonObject(with: data, options: .allowFragments) as? [String: Any] else { | ||
return [:] | ||
} | ||
|
||
return dictionary | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(의견)
요 함수는 TargetType 내부에서 Self.로 접근하는것 같은데, static이 아닌 private func으로 구현해도 될 것 같네요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24.10.28 (iOS 회의 결과)
- static 제거
case .postSignup: | ||
return "/auth/signup" | ||
case .postSignin: | ||
return "/auth/login" | ||
case .getCheckUsername, .getCheckEmail: | ||
return "/auth/users" | ||
case .postUpdatePassword: | ||
return "/auth/users/password" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(의견)
AuthAPI로 첫 path명과 동일하게 API 라우터 만들어 주신건 너무 좋은것 같습니다!
개인적인 추가 의견으로 API case와 path를 통일하면 좋겠다는 의견입니다,
API가 많아지다 보면 호출하는곳(사용하는곳)이 아닌 해당 파일의 path까지 와서 확인해야만 어떤 api인지 알 수 있어 이슈 디버깅하는 속도가 느려질것 같아요
회의때 의견 나누면 좋을것 같아서 코멘트 남깁니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24.10.28 (iOS 회의 결과)
- API path에 대한 컨벤션(?)을 백엔드와 논의해보자
- 우리가 path를 따라서 네이밍을 하고 있다는것 정도만 언급하기
@@ -17,19 +19,9 @@ enum NetworkError: Error { | |||
case NOTFOUND(String?) // 404 | |||
case CONFLICT(String?) // 409 | |||
case SERVERERROR(String?) // 500 | |||
case ERRORRESPONSE(TTErrorResponse) // TiTi ErrorResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(질문)
API case를 uppercase로 작성하신 이유가 있을까요?
swift의 네이밍 컨벤션과 맞지 않는것 같아서 질문 드립니당
개인적으론 ERRORRESPONSE의 경우 uppercase가 직관적이지 않은것 같아요,,
(의견) -> errorResponse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 훑어 봤습니다! 궁금증과 의견 남깁니다
// TODO: DI 수정
요거는 라이브러리 반영 예정이라는건지, DI 방식 변경을 의미하는건지 ?
추가로 위 DI를 VC에서 직접 하고 그러는것 같은데,, 그래서 TODO를 써두신것 같다는 생각이 드네용
불필요ㅏㄴ dependency가 너무 강해지는 것 같아서 어디다 빼서 작성하는 로직을 한 번 같이 고민해보시죵!
final class AuthRepository { | ||
private let api: TTProvider<AuthAPI> | ||
|
||
func signup(signupInfo: TestUserSignupInfo, completion: @escaping (Result<AuthInfo, NetworkError>) -> Void) { | ||
api.signup(signupInfo: signupInfo) { result in | ||
switch result.status { | ||
case .SUCCESS: | ||
guard let data = result.data, | ||
let dto = try? JSONDecoder().decode(AuthDTO.self, from: data) else { | ||
completion(.failure(.DECODEERROR)) | ||
return | ||
} | ||
|
||
let info = dto.toDomain() | ||
completion(.success(info)) | ||
|
||
default: | ||
completion(.failure(.error(result))) | ||
} | ||
} | ||
init(api: TTProvider<AuthAPI>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(의견)
모든 Repository에 api를 주입 해주는것 같아요,
근데 api가 추상화가 아니라 구현체라 굳이 주입을 받아야 하는지 의문이 들어요
추상화 또는 내부에서 생성하는 방향을 잡으면 좋을듯 합니당
print("\nTTProvider success", token, "\(token.baseURL)\(token.path)") | ||
// print("-->", String(data: response.data, encoding: .utf8), "\n") | ||
if (200...299).contains(response.statusCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 주석은 특별히 남겨 주신 이유가 없다면 제거 부탁드립니다〰️
print("\nTTProvider failure", token, "\(token.baseURL)\(token.path)") | ||
// print("-->", error.localizedDescription, "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 주석은 특별히 남겨 주신 이유가 없다면 제거 부탁드립니다〰️
private let repository: AuthRepository // TODO: 프로토콜로 수정 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(질문)
요거 Repository 전체적으로 TODO 상태인것 같은데 아직 반영 안되는 이유가 있을까여?
리뷰 요청
개요
참고 : 너무 변경사항이 많아서... 추후 PR 내용에 반영 후 다시 공유드리겠습니다..! 🥲
변경사항
작업 요약
특이 사항
Network Layer 흐름도
추가 및 변경된 API 요청 UseCase
각 API 에 따라 UseCase, Request, Response, Info, Repository, API 등이 생성되었습니다.
예시 코드
GetDailysUseCase (SyncDailysVM)
리뷰 받고 싶은 내용