Feature/js network formdata type by jsanderson44 · Pull Request #71 · theappbusiness/TABResourceLoader

jsanderson44 · 2018-01-30T13:08:31Z

Added NetworkFormDataResourceType and NetworkJSONResourceType.
Refactored NetworkResourceType to take more generic 'body' and moved encoding of data to the conforming types.
Updated tests

Refactored NetworkResourceType to take more generic 'body' and moved encoding of data to the conforming types. Updated tests

codecov-io · 2018-01-30T13:11:39Z

Codecov Report

Merging #71 into master will increase coverage by 0.43%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   86.12%   86.56%   +0.43%     
==========================================
  Files          22       22              
  Lines         346      268      -78     
  Branches       30        0      -30     
==========================================
- Hits          298      232      -66     
+ Misses         48       36      -12

samdods · 2018-01-30T15:54:25Z

Sources/TABResourceLoader/Extensions/Dictionary+Additions.swift

+    self.forEach { (key, value) in
+      data.append(key + "=\(value)")
+    }
+    return data.map { String($0) }.joined(separator: "&")


you could use the syntax return data.map(String.init).joined(separator: "&")

samdods · 2018-01-30T15:55:07Z

Sources/TABResourceLoader/Extensions/Dictionary+Additions.swift

+  /// Returns dictionary as string used to POST Form Data
+  public var formDataPostString: String {
+    var data = [String]()
+    self.forEach { (key, value) in


superfluous self

samdods · 2018-01-30T16:35:58Z

Sources/TABResourceLoader/Extensions/Dictionary+Additions.swift

+
+import Foundation
+
+public extension Dictionary where Key == String, Value == Any {


presumably it's just left to the consumer to ensure their value type can be converted to a string? only works for primitive types.

samdods · 2018-01-30T20:03:08Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkResourceType.swift

-  /// The HTTP body as JSON used to fetch this resource
-  var jsonBody: Any? { get }
+  /// The HTTP body used to fetch this resource
+  var body: Any? { get }


this isn't needed by this protocol. it's only used by the protocols that inherit from this protocol. i suggest declaring it specifically in each of the other protocols, which also means you can name it more appropriately for each of the protocols that actually uses it.

it could lead to confusion. e.g. if you create a concrete conformer of NetworkResourceType (that does not conform to either NetworkJSONResourceType or NetworkPropertyListDecodableResourceType), then setting the body property does nothing.

samdods · 2018-01-30T20:05:24Z

...es/TABResourceLoader/Protocols/Resource types/NetworkPropertyListDecodableResourceType.swift

  var httpHeaderFields: [String: String]? {
    return ["Content-Type": "application/x-plist"]
  }
+


i suggest adding var jsonBody: Any? { get } to the protocol definition. this demands that your conforming concrete type provides this property, and the property name is more descriptive of how it will be used.

actually, this plist one is a bit of a mess. it's intended to parse a plist from the response. the type being sent in the request is JSON, as you've created below. but the Content-Type header field is "application/x-plist" - that's wrong. we should:

either change the Content-Type to "application/json"

change the transformation from body to bodyData to use PropertyListSerialization instead of JSONSerialization - although this is probably ludicrous

change the entire protocol conformance pattern to distinguish between incoming and outgoing data formats (e.g. what if you want to send form data, but get a JSON response?)

samdods · 2018-01-30T20:11:26Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkJSONResourceType.swift

+import Foundation
+
+/// Defines a resource that can be fetched from a network where the root type is a JSON array
+public protocol NetworkJSONResourceType: NetworkResourceType {}


i suggest adding var jsonBody: Any? { get } to the protocol definition. (see my previous comment above removing the body property from the NetworkResourceType definition.

deanbrindley87 · 2018-07-10T10:39:10Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkJSONResourceType.swift

+
+import Foundation
+
+/// Defines a resource that can be fetched from a network where the root type is a JSON array


not sure this comment is true john, what do you mean by root type? i don't believe this resource specifies what type it is expecting in the JSON body response, only the request, and that's not specified as an array anyway

Updated. Now in line with NetworkFormDataResourceType docs

NikolaKirev · 2018-07-10T10:38:34Z

Sources/TABResourceLoader/Extensions/Dictionary+Additions.swift

+//  TABResourceLoader
+//
+//  Created by John Sanderson on 30/01/2018.
+//  Copyright © 2018 Luciano Marisi. All rights reserved.


Maybe use the MIT license and the TAB copyright here.

NikolaKirev · 2018-07-10T10:46:35Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkJSONResourceType.swift

+//
+//  The MIT License (MIT)
+//
+//  Copyright (c) 2016 Luciano Marisi


Maybe change the copyright ot TAB here.

NikolaKirev · 2018-07-10T10:49:43Z

Tests/TABResourceLoaderTests/Protocols/NetworkJSONResourceTypeTests.swift

+//  TABResourceLoader
+//
+//  Created by John Sanderson on 30/01/2018.
+//  Copyright © 2018 Luciano Marisi. All rights reserved.


Maybe use the MIT license and the TAB copyright here.

NikolaKirev · 2018-07-10T10:52:40Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkFormDataResourceType.swift

+    return ["Content-Type": "application/x-www-form-urlencoded"]
+  }
+
+  var bodyData: Data? {


Does it make sense to name this httpBody or just body?

yeah i like that idea @NikolaKirev , people will automatically start typing http if you want to add data to the body of your request so it'll help in xcode

hm there is also formDataBody, that could be quite confusing in the resource which one you should be using

Have updated to httpBody

NikolaKirev · 2018-07-10T10:55:10Z

Sources/TABResourceLoader/Extensions/Dictionary+Additions.swift

+
+  /// Returns dictionary as string used to POST Form Data
+  public var formDataPostString: String {
+    var data = [String]()


Naming this data might be confusing. How about formEntries or entries?

NikolaKirev · 2018-07-10T10:58:09Z

Sources/TABResourceLoader/Extensions/Dictionary+Additions.swift

+      let stringValue = String(describing: value)
+      data.append(key + "=" + stringValue)
+    }
+    return data.map(String.init(_:)).joined(separator: "&")


Why is the map(String.init(_:)) needed? Could the whole thing be done with a single reduce call on the dictionary?

yeah a single reduce would work better. but anyway data is of type [String] so data.map(String.init) will just return the same as data. won't it?

Updated to use reduce

deanbrindley87 · 2018-07-10T11:06:45Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkJSONDictionaryResourceType.swift

-    return ["Content-Type": "application/json"]
-  }
-}
+public protocol NetworkJSONDictionaryResourceType: NetworkJSONResourceType, JSONDictionaryResourceType {}


i'm not so sure about this, i feel like we need a bit of a beat up around it. it's not clear from the name that the request body needs to be JSON (if you need to send data) and also that the response type is JSON. seems like this was an existing issue in TRL and why we needed this PR in the first place. maybe we need to look at removing some of these types and leave it to the consumer to compose the resource types they want (that would obviously be a breaking change 😢 ). i also think it might be nice to make a clearer distinction between the resource request information (NetworkJSONResourceType) and the resource response information (JSONDictionaryResourceType). i find the namings all a bit confusing at the moment... but i could just be out of touch as i didn't build this to start with. @Neil3079 @lucianomarisi any thoughts?

as a side note i think the change in NetworkResourceType.swift is spot on and where we should be heading for

i was thinking and commenting exactly the same @deanbrindley87. see my comments 😄

samdods · 2018-07-10T11:10:56Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkJSONResourceType.swift

+  var jsonBody: Any? { return nil }
+
+  var httpHeaderFields: [String: String]? {
+    return ["Content-Type": "application/json"]


i've never actually liked the fact that we had this in there in the first place. i'm sure most users of this library are sending GET requests without sending content.

it would be better to distinguish between the request content type and the response content type.

e.g. would you ever need to send FORM data and get a JSON response? perhaps. the current implementation restricts this by confusing the two. this isn't anything to do with your change, just an observation about the restriction of the library in the first place. we've definitely gone with the simplest approach, which serve 90% of use cases. but something to bear in mind.

on second thoughts, i think it would be good to give this more thought now.

looking at your NetworkFormDataResourceType, it conforms to NetworkResourceType and DataResourceType. i assume you just use this to send form data and get an HTTP response code without any content in the response body?

what if you did get some content in the response body? what about error messages? do you get any JSON to give you more details on the error?

i think it would be good to think about this in terms of DataRequestType and DataResponseType protocols, which we could extend with FormDataRequestType, JSONDataRequestType, FormDataResponseType and FormDataResponseType. do you ever get form data in a response?

the "Request" type protocols would implement in protocol extensions the var body: Data? { get } requirement of the DataRequestType protocol, just as your NetworkJSONResourceType extension does here. these extensions would also implement the relevant header fields' content type.

the "Response" type protocols would implement in protocol extensions the func model(from data: Data) throws -> Model requirement of the DataResponseType protocol, just as the JSONDictionaryResourceType currently does.

@samdods did you read my comment above? looks like we're on a similar page...

yeah just read your comment after writing mine 👍

deanbrindley87 · 2018-07-10T11:15:08Z

Sources/TABResourceLoader/Protocols/Resource types/NetworkFormDataResourceType.swift

+    return ["Content-Type": "application/x-www-form-urlencoded"]
+  }
+
+  var httpBody: Data? {


i'll check the code out and have a look @jsanderson44 , not following enough on here!!

samdods

as per comments. let's discuss

John Sanderson added 2 commits January 30, 2018 12:59

Added NetworkFormDataResourceType and NetworkJSONResourceType.

f4d74f0

Refactored NetworkResourceType to take more generic 'body' and moved encoding of data to the conforming types. Updated tests

Moved dictionary extension to its own file. Added test

de963da

jsanderson44 requested review from deanbrindley87 and samdods January 30, 2018 13:08

samdods reviewed Jan 30, 2018

View reviewed changes

John Sanderson added 4 commits January 31, 2018 11:31

Addressed some PR comments

a883fab

added default value

f5ca727

Made formDataBody [String: Any]

6cba9e2

updated FormData type to use CustomStringConvertible

658fff3

deanbrindley87 reviewed Jul 10, 2018

View reviewed changes

Updated docs

b760d32

NikolaKirev reviewed Jul 10, 2018

View reviewed changes

deanbrindley87 reviewed Jul 10, 2018

View reviewed changes

samdods reviewed Jul 10, 2018

View reviewed changes

Addressed Nikolas PR comments. Changed dictionary helper to use reduce

d3a2284

deanbrindley87 reviewed Jul 10, 2018

View reviewed changes

samdods suggested changes Jul 10, 2018

View reviewed changes

MarcoGuerrieriTAB changed the base branch from master to main September 15, 2020 09:41


		import Foundation

		public extension Dictionary where Key == String, Value == Any {


		import Foundation

		/// Defines a resource that can be fetched from a network where the root type is a JSON array

Conversation

jsanderson44 commented Jan 30, 2018

Uh oh!

codecov-io commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikolaKirev Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdods left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jan 30, 2018 •

edited

Loading

NikolaKirev Jul 10, 2018 •

edited

Loading