[Refactor] NearbyNetwork 광고, 검색 기능 리팩터링 #148
Conversation
| var reciptDataPublisher: AnyPublisher<Data, Never> { | ||
| return Just<Data>(Data()).eraseToAnyPublisher() | ||
| } | ||
|
|
||
| var reciptURLPublisher: AnyPublisher<(url: URL, dataInfo: DataInformationDTO), Never> { | ||
| // TODO: - will be deprecated | ||
| guard let url = URL(string: "https://naver.com") else { fatalError() } | ||
| let dataInfo = DataInformationDTO( | ||
| id: UUID(), | ||
| type: .chat, | ||
| isDeleted: true) | ||
| return Just<(url: URL, dataInfo: DataInformationDTO)>((url: url, dataInfo: dataInfo)) | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
이후에 세부 내용 변경되면서, DTO init을 public으로 변경한 부분도 같이 지워지나요?
There was a problem hiding this comment.
아마 그럴것 같은데요, 이 부분은 추후 변경되면 노티해드리겠습니다!
| let connectionData = [ | ||
| NearbyNetworkKey.host.rawValue: hostName, | ||
| NearbyNetworkKey.connectedPeerInfo.rawValue: connectedPeerInfo.joined(separator: ",")] | ||
| let txtRecord = NWTXTRecord(connectionData) |
There was a problem hiding this comment.
txtRecord로 이후 누가 들어와있는지 publishing할 계획이군요..
NWTXTRecord타입을 따로 제공해주니 좋네요
There was a problem hiding this comment.
저 부분 덕분에 삽질을 정말 많이 했습니다 😭😭
갓갓 조이 덕분에 슬기롭게 해결할 수 있었습니다!! 완전 동의하는 부분~
| import OSLog | ||
|
|
||
| final class NearbyNetworkBrowser { | ||
| let nwBrowser: NWBrowser |
There was a problem hiding this comment.
외부에서 browser를 직접 접근 하는 경우가 있을까요?
There was a problem hiding this comment.
이런,,따로 외부에서 browser에 접근하는 경우가 없기 때문에 private으로 바꾸도록 하겠습니다!!
| break | ||
| } | ||
| } | ||
| wait(for: [browsingExpectation], timeout: 3) |
There was a problem hiding this comment.
비기능적 요구사항이 될 것 같은데요, 일단은 주관적으로 3초 정도면 괜찮지 않을까하여 설정했습니다
There was a problem hiding this comment.
좋아요!
더 생각해 봤는데, 프레임워크의 기능인 네트워킹 자체를 테스트하는 거 보다도 저희가 구현한 부분인 (어떤 내용으로 �광고 하는지 등을)테스트 하는 것이라는 생각이 들었습니다!
그렇기에 몇초로 타임아웃을 두든지 네트워킹이 진행 될 수 있는 충분한 시간이면 될 것 같다는 생각이 드네요 :)
| public init( | ||
| id: UUID, | ||
| name: String, | ||
| connectedPeerInfo: [String] | ||
| ) { | ||
| self.id = id | ||
| self.name = name | ||
| self.connectedPeerInfo = connectedPeerInfo | ||
| } | ||
| } | ||
|
|
||
| extension RefactoredNetworkConnection: Equatable { | ||
| public static func == (lhs: RefactoredNetworkConnection, rhs: RefactoredNetworkConnection) -> Bool { | ||
| return lhs.id == rhs.id | ||
| } |
There was a problem hiding this comment.
아직 이부분이 필요한 곳을 찾지 못했는데, 어떻게 사용되는 걸까요?
There was a problem hiding this comment.
우선 이름 앞에 Refactored 라는 단어가 붙으면, 기존 타입을 대체할 새로운 타입으로 봐주시면 감사하겠습니다!
그럼 Equatable과 함께 Codable에 대한 설명을 같이 드리겠습니다.
기존 계획은 NWTXTRecord 부분을 Data 형식으로 생성이 가능하길래 아래와 같은 방식을 통해 advertising 하려 했습니다.
- RefactoredNetworkConnection 인스턴스를 JSONEncoder를 통해 JSON data로 인코딩
- 인코딩된 data로 NWTXTRecord 를 생성
- NWListener( 이 녀석이 advertising 해주는 녀석입니다.)의 service에 생성한 NWTXTRecord정보를 넣음
- NWListener를 통해 광고
하지만 browsing 할 때 정상적으로 NWListener의 service에 넣어준 NWTXTRecord를 찾지 못하는 문제가 있었습니다.
디버깅을 해보니 NWTXTRecord를 초기화 할 때 아무 Data나 넣어서는 정상적으로 생성이 안되는 문제를 발견했습니다. (NWTXTRecord를 생성할 수 있는 형식에 맞는 Data가 필요한듯)
그래서 NWTXTRecord를 Data를 이용한 방식이 아닌, [String: String] 형식의 딕셔너리로 생성하는 방법을 채택했습니다.
정리하자면, 기존에는 Advertising을 할 때 RefactoredNetworkConnection 형식 자체를 넣어서 광고하려 했는데, 이 방법을 사용할 수 없겠다~가 된 것이죠. 그래서 아마 Codable 채택은 빠지게 될 것 같습니다.
다만 Equatable의 경우 현재 NetworkConnection이 Equatable을 채택하고 있고, 이를 통해 각 connection을 관리할 수 있는 것으로 알고 있습니다. 따라서 추후 RefactoredNetworkConnection도 같은 방식으로 connection간 비교할 때 사용되지 않을까 하여 채택해두었습니다. (기존 NetworkConnection을 바로 대체할 수 있도록 함)
There was a problem hiding this comment.
확인 했습니다 :)
플로우 설명 감사합니다ㅎㅎ
확실히 새로운 프레임워크를 사용하다보니 삽질하는 경우가 많네요.. 고생많으셨습니다ㅜㅜ
| browser.start(queue: browserQueue) | ||
| browser.browseResultsChangedHandler = { results, _ in | ||
| for result in results { | ||
| switch result.metadata { | ||
| case .bonjour(let foundedPeerData): | ||
| browsedHostName = foundedPeerData.dictionary[NearbyNetworkKey.host.rawValue] | ||
| browsedPeerInfo = foundedPeerData.dictionary[NearbyNetworkKey.connectedPeerInfo.rawValue] | ||
| browsingExpectation.fulfill() | ||
| default: | ||
| break | ||
| } | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
아래 테스트와 마찬가지로, 테스트하기 위해 필요한 부분이니까 mock으로 빼는 건 어땠을까요??
There was a problem hiding this comment.
오 이게 훨씬 깔끔한 방법이라 생각합니다!! 아주 굿굿!!!!
타임 어택을 하다보니,, 개선할 부분이 많군요
There was a problem hiding this comment.
아닙니다 고생 많으셨어요!!
여유있는 제 3자입장에서 도움 줄 수 있는 방법을 생각하다보니 나오는 것들인 것 같아요!!
| public init( | ||
| id: UUID, | ||
| name: String, | ||
| connectedPeerInfo: [String] | ||
| ) { | ||
| self.id = id | ||
| self.name = name | ||
| self.connectedPeerInfo = connectedPeerInfo | ||
| } | ||
| } | ||
|
|
||
| extension RefactoredNetworkConnection: Equatable { | ||
| public static func == (lhs: RefactoredNetworkConnection, rhs: RefactoredNetworkConnection) -> Bool { | ||
| return lhs.id == rhs.id | ||
| } |
taipaise
force-pushed
the
refactor/NearbyNetwork
branch
2 times, most recently
from
e7a6460 to
6962f0d
Compare
🌁 Background
MPC Framework를 Network Framework로 교체 작업을 시작하였습니다.
다음과 같은 순서로 리팩터링 진행 예정입니다.
👩💻 Contents
이번 PR에서는 2번까지 작업 완료하였습니다.
✅ Testing
NearbyNetworkTest를 통해 광고, 검색을 테스트 하였습니다.
📝 Review Note
Browsing을 테스트 하기 위해 NearbyNetworkService의 구조를 변경했습니다.
검색된 connection에 대한 정보를 검증하기 위해 NearbyNetworkService에 foundPeerHandler를 추가하였습니다.
추후 참여 구현 과정에서 수정될 수 있습니다.
📣 Related Issue