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.

Sign up for GitHub

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

Jump to bottom

chore: 변경된 서버 API 필드명으로 회원가입 여부 DTO 변경, 코드 리뷰 반영 #223

Merged
merged 1 commit into from
Dec 6, 2023
Merged

chore: 변경된 서버 API 필드명으로 회원가입 여부 DTO 변경, 코드 리뷰 반영 #223

merged 1 commit into from
Dec 6, 2023

Conversation

loinsir
Copy link
Collaborator

@loinsir loinsir commented Dec 6, 2023

🧑‍🚀 PR 요약

  • 변경된 서버 API 필드명 isAlreadyExist로 회원가입 여부 DTO 필드명을 변경했습니다.

📌 변경 사항

  • MockURLProtocol.requestHandler data race 문제
    • async let을 이용한 병렬 실행으로 카카오 로그인 로직을 애플과 동시에 맞춰주려고 했는데, Mock 메서드 내에서 실행되는 MockURLProtocol.requestHandler 설정부분에서 data race되어 decoding error가 발생하는 이슈가 있었습니다.
    • 당장은 고치려면 Mock쪽을 건드려야 하게 되어서 우선은 각각을 await하는 것으로 변경했습니다.

Linked Issue

close #222

@loinsir loinsir self-assigned this Dec 6, 2023
@loinsir loinsir linked an issue Dec 6, 2023 that may be closed by this pull request
chore: 회원가입 여부 체크 DTO 필드 변경 #222
Closed
1 task
anyukyung
anyukyung approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@anyukyung anyukyung left a comment

Choose a reason for hiding this comment

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

병렬 실행했을 때 그런 이슈가 있을 수 있겠네요,,
원하시는 방향대로 하려면 모든 Mock Worker에서 data race 발생하지 않도록 처리해주어야 하려나요? 🤔
암튼간에 수고하셨습니당..

@loinsir
Copy link
Collaborator Author

loinsir commented Dec 6, 2023

병렬 실행했을 때 그런 이슈가 있을 수 있겠네요,, 원하시는 방향대로 하려면 모든 Mock Worker에서 data race 발생하지 않도록 처리해주어야 하려나요? 🤔 암튼간에 수고하셨습니당..

MockURLProtocol에서 requestHandler를 static var로 설정해서 넣는것이 아니라, 각각 독립적으로 설정할 수 있도록 변경해야 합니다.
이건 이슈주제랑 너무 벗어나는 것 같아서 우선은 저렇게 처리했어요 ㅠ

chopmozzi
chopmozzi approved these changes Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@chopmozzi chopmozzi left a comment

Choose a reason for hiding this comment

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

확인 완료!

iOS/Layover/Layover/Scenes/Login/LoginInteractor.swift
Comment on lines -80 to +81
async let isRegistered: Bool = worker?.isRegisteredApple(with: identityToken) ?? false
async let loginResult: Bool = worker?.loginApple(with: identityToken) ?? false
let isRegistered = await worker?.isRegisteredApple(with: identityToken)
let loginResult = await worker?.loginApple(with: identityToken)
Copy link
Collaborator

Choose a reason for hiding this comment

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

전에 그 병렬 처리 코드네요 확인했습니다.

Copy link
Collaborator Author

@loinsir loinsir Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Mock으로 하니까 여기도 간헐적으로 오류가 나네요 ㅠ 실 Worker로 하면 원래 코드는 오류 나지 않을 거에요

@loinsir loinsir merged commit d702bc6 into iOS/dev Dec 6, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anyukyung anyukyung anyukyung approved these changes

@chopmozzi chopmozzi chopmozzi approved these changes

Assignees

@loinsir loinsir

Labels
🧹 chore 🍎 iOS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore: 회원가입 여부 체크 DTO 필드 변경
3 participants
@loinsir @chopmozzi @anyukyung