Overhaul and deprotocolize Network 🛰 #218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mickael-domingues
approved these changes
Jun 2, 2020
beloso
reviewed
Jun 4, 2020
beloso
reviewed
Jun 4, 2020
beloso
approved these changes
Jun 4, 2020
|
I like these changes. It reduces complexity on the network stack implementation. |
nqtavares
approved these changes
Jun 5, 2020
ricardomsm
approved these changes
Jun 16, 2020
As it was, our `NetworkStack` and it's main conforming type `URLSessionNetworkStack` were not straightforward to use, mostly because of the many different `Resource` protocols and respective associated types. All these `Resource` protocols, while being broken into simple, focused and composable units, created lots of complexity due to the generic constraints they impose. While initially designed for allowing diverse `NetworkStack`'s to be modeled, we have observed that most of the times users are simply interested in using a `URLSessionNetworkStack`. The `NetworkStore` suffered from similar issues, by being tied to our huge `Resource` protocol family, while its main idea (and sole implementation) was a store that orchestrated both a network and persistence stacks. Finally, `PersistenceStack` received a minor clean up and improvement to make it more flexible and powerful. With all these things in mind, our `Resource` "family" was rethought to be composed of multiple witnesses, instead of multiple protocols. Witnesses provide the same abstraction of modeling behavior, without some of the limitations and complexities of protocols. By being generic concrete types, they also allow creating default implementations via static properties and functions, making code reuse very simple. The `NetworkStack` protocol was kept but greatly simplified and the `NetworkStore` protocol was discontinuated in favor of a `StackOrchestratorStore`, also simplified while making its purpose clearer. ## Changes - Deprecate all `Resource` protocols. - Create new `BaseRequestMaking` witness to replace `BaseRequestResource`. - Create new `ModelDecoding` witness to replace `DecodableResource`. - Create new `ErrorDecoding` witness to replace `ExternalErrorDecoderResource`. - Deprecate `RequestAuthenticator` in favor of `URLRequestAuthenticator`, providing a simpler API. - Deprecate `RequestInterceptor` in favour of a more powerful `URLSessionResourceInterceptor` defined at a Resource level, allowing much more flexibility and control. Add default conformances to `URLRequestAuthenticator`, `Network.URLSessionRetryPolicy`. - Improve `Retry` infrastructure: + Create new `Retry.State` to model the retry state of an arbitrary operation, update `Policy.shouldRetry()` to use it. + Create `Retry.Action.CompareClosure` to compare actions. + Create `Retry.Action.mostPrioritary` helper static function, with the default logic to pick the most prioritary action between two. + Rename `Retry.Policy.retries` to `maxRetries`. + Rename `Retry.Policy.Backoff.Truncation.retries` to `maxRetries`. + Rename `Retry.Policy.Backoff.Truncation.delay` to `maxDelay`. - Improve `HTTPResourceEndpoint`: + Convert `body` property into a throwable function, to allow handling enconding errors. + Convert `request` computed property into a throwable function, to propagate body encoding errors. - Rename `Network.URLSessionNetworkStack.Error` to `Network.URLSessionError` and improve it: + Remove `URLResponse` associated value from `.url` and `.retry` + Use new `Retry.State` - Update `NetworkStack` to require just a `Resource` and `FetchError` associated types, and add protocol extension to perform decoding after a network fetch. - Update Network playground with recent changes. - Improve `PersistenceStack` to have a new `Key: Hashable` `associatedtype` instead of the hardcoded `Persistence.Key`, and renamed existing `Remote` to `Payload`. - Deprecate `NetworkStore` in favor of `StackOrchestratorStore`, which requires a `NetworkStack` and `PersistenceStack associated types. Add a protocol extension to perform decoding after a fetch. - Create new `StackOrchestrator` namespace. - Rename `NetworkPersistableStore` to `StackOrchestrator.Store`, plus some internal adjustments. - Rename `NetworkStorePerformanceMetricsTracker` to `StackOrchestratorPerformanceMetricsTracker`. - Update `podspec`. - Add some new rule exceptions on `.swiftlint.yml`
- Remove `Network.URLSessionNetworkStack.Configuration` - Create `FetchAndDecodeError`. - Move `ModelDecoding` out of `Network` namespace. - Improve default decoding witnesses helpers.
p4checo
force-pushed
the
feature/overhaul-and-deprotocolize-network
branch
from
3d0856a
to
fecb2ea
Compare
p4checo
requested review from
ivopintodasilva,
beloso,
nqtavares,
ricardomsm and
a team
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
==========================================
+ Coverage 94.81% 94.94% +0.12%
==========================================
Files 100 102 +2
Lines 3243 3345 +102
==========================================
+ Hits 3075 3176 +101
- Misses 168 169 +1
Continue to review full report at Codecov.
|
ricardomsm
approved these changes
Sep 21, 2020
ivopintodasilva
approved these changes
Sep 22, 2020
diogoeiras
suggested changes
Sep 22, 2020
diogoeiras
approved these changes
Sep 22, 2020
filipe-lemos
approved these changes
Sep 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
masterMotivation and Context
As it was, our
NetworkStackand it's main conforming typeURLSessionNetworkStackwere not straightforward to use, mostly because of the many differentResourceprotocols and respectiveassociated types. All these
Resourceprotocols, while being broken into simple, focused and composable units, created lots of complexity due to the generic constraints they impose.While initially designed for allowing diverse
NetworkStack's to be modeled, we have observed that most of the times users are simply interested in using aURLSessionNetworkStack.The
NetworkStoresuffered from similar issues, by being tied to our hugeResourceprotocol family, while its main idea (and sole implementation) was a store that orchestrated both a network and persistence stacks.Finally,
PersistenceStackreceived a minor clean up and improvement to make it more flexible and powerful.With all these things in mind, our
Resource"family" was rethought to be composed of multiple witnesses, instead of multiple protocols. Witnesses provide the same abstraction of modeling behavior, without some of the limitations and complexities of protocols. By being genericconcrete types, they also allow creating default implementations via static properties and functions, making code reuse very simple.
The
NetworkStackprotocol was kept but greatly simplified and theNetworkStoreprotocol was discontinuated in favor of aStackOrchestratorStore, also simplified while making its purpose clearer.Some concepts discussed on the amazing pointfree's videos were used, in particular protocol witnesses. Feel free to check this talk or their video series over at pointfree.co. 🤓🙏
Description
Deprecate all
Resourceprotocols, and replace most relevant ones with witnesses:Create new
BaseRequestMakingwitness to replaceBaseRequestResource.Create new
ModelDecodingwitness to replaceDecodableResource.Create new
ErrorDecodingwitness to replaceExternalErrorDecoderResource.Deprecate
RequestAuthenticatorin favor ofURLRequestAuthenticator, providing a simpler API.Deprecate
RequestInterceptorin favour of a more powerfulURLSessionResourceInterceptordefined at a Resource level, allowing much more flexibility and control. Add default conformances toURLRequestAuthenticator,Network.URLSessionRetryPolicy.Improve
Retryinfrastructure via newRetry.State,Retry.Action.CompareClosureandRetry.Action.mostPrioritaryhelper static function. RenameRetry.Policy.retriestomaxRetries,Retry.Policy.Backoff.Truncation.retriestomaxRetriesandRetry.Policy.Backoff.Truncation.delaytomaxDelay.Improve
HTTPResourceEndpoint:Convert
bodyproperty into a throwable function, to allow handling encoding errors.Convert
requestcomputed property into a throwable function, to propagate body encoding errors.Rename
Network.URLSessionNetworkStack.ErrortoNetwork.URLSessionErrorand refine it by removing unnecessaryURLResponseand using newRetry.State.Update
NetworkStackto require just aResourceandFetchErrorassociated types, and add protocol extension to perform decoding after a network fetch.Update Network playground with recent changes.
Improve
PersistenceStackto have a newKey: Hashableassociatedtypeinstead of the hardcodedPersistence.Key, and renamed existingRemotetoPayload.Deprecate
NetworkStorein favor ofStackOrchestratorStore, which requires aNetworkStackand `PersistenceStack associated types. Add a protocol extension to perform decoding after a fetch.Create new
StackOrchestratornamespace.Rename
NetworkPersistableStoretoStackOrchestrator.Store, plus some internal adjustments.Rename
NetworkStorePerformanceMetricsTrackertoStackOrchestratorPerformanceMetricsTracker.Update
podspec.Add some new rule exceptions on
.swiftlint.yml