-
Notifications
You must be signed in to change notification settings - Fork 25
fix: remove APIVersionResolver - WPB-22732 #4132
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
base: develop
Are you sure you want to change the base?
Conversation
Test Results 4 files 511 suites 3m 47s ⏱️ For more details on these failures, see this check. Results for commit ce035b5. ♻️ This comment has been updated with latest results. |
…wireapp/wire-ios into fix/WPB-22732-remove-apiversionresolver
| private func configureTransportSession() { | ||
| transportSession.setNetworkStateDelegate(self) | ||
| transportSession.setAccessTokenRenewalFailureHandler { [weak self] response in | ||
| self?.transportSessionAccessTokenDidFail(response: response) |
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.
question: are you sure we can delete this? It seems like it's necessary to trigger logout.
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.
not completely sure 😅, i thought it was only for renewing token after api v2
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.
reverted
samwyndham
left a comment
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.
Nice work!
| if let apiVersion { | ||
| self = .apiVersion(apiVersion) | ||
| init(apiVersion: WireNetwork.APIVersion?) { | ||
| if let apiVersion, let version = WireTransport.APIVersion(rawValue: Int32(apiVersion.rawValue)) { |
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.
thought (maybe out of scope): I guess we should always be able to map a WireNetwork.APIVersion to WireTransport.APIVersion right? if nil is returned here it doesn't really mean no preference but is some programmer error. I think it is worth adding mappers somewhere that convert between the two APIVersion types.
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.
yeah for me the fix is to remove duplication, it should be doable either in making WireNetwork a dependency of WireTransport or moving APIVersion definition to WireFoundation.
I guess the latter is more appropriate
Issue
Following removing dead code from non multibackend code, this PR removes the APIVersionResolver
Testing
N/A
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: