Fix request body creator non-default implementations getting hammered by compiler #1450
Conversation
| @@ -69,7 +69,10 @@ open class UploadRequest<Operation: GraphQLOperation>: HTTPRequest<Operation> { | |||
| // Make sure all fields for files are set to null, or the server won't look | |||
| // for the files in the rest of the form data | |||
| let fieldsForFiles = Set(files.map { $0.fieldName }).sorted() | |||
| var fields = self.requestBodyCreator.requestBody(for: operation, sendOperationIdentifiers: shouldSendOperationID) | |||
| var fields = self.requestBodyCreator.requestBody(for: operation, | |||
There was a problem hiding this comment.
This has to change due to the removal of the default values for these parameters
There was a problem hiding this comment.
Hi @designatednerd , although the default values for these parameters has been removed, but I think it's a good idea to add a new extension method like this:
/// Don't override this method, override `requestBody(operation, false, true, false)`
extension RequestBodyCreator {
public func requestBody<Operation: GraphQLOperation>(for operation: Operation) {
requestBody(operation, false, true, false)
}
}
There was a problem hiding this comment.
I'll consider that, but I am concerned that would run a significant risk of reintroducing this same problem.
| @@ -270,7 +270,10 @@ public class WebSocketTransport { | |||
| } | |||
|
|
|||
| func sendHelper<Operation: GraphQLOperation>(operation: Operation, resultHandler: @escaping (_ result: Result<JSONObject, Error>) -> Void) -> String? { | |||
| let body = requestBodyCreator.requestBody(for: operation, sendOperationIdentifiers: self.sendOperationIdentifiers) | |||
| let body = requestBodyCreator.requestBody(for: operation, | |||
There was a problem hiding this comment.
This has to change due to the removal of the default values for these parameters
|
Took a second pass after a day and discovered the real best way to handle this was to just ditch the default values in the default implementation. Made a couple tests dumber, but I think from a standpoint of not breaking as hard, this is better. Still technically breaking since I removed the default values, though. |
Addresses #1444. Note that this will be a breaking change.
So I seriously had to rewatch this bit of my own talk about this to figure out why this hadn't twigged on me earlier: I was expecting this to be like the
sayHimethod in that example, where because it's declared in the protocol, the compiler uses the lowest level of implementation rather than going to to the default implementation. Instead, it's behaving like theaskHowAreYoumethod, which is not defined in the implementation, and which is defaulting to the protocol's default implementation.The test we had previously validated that things worked when called directly, but not when called indirectly: When I made the changes to the test in this PR, it immediately started failing, so that definitely meant that we were closer to the
askHowAreYousituation.As far as I can tell, this is some weird combination of circumstances in terms of what we were doing with default values on the default argument vs. the implementation in the test. I suspect that because it looked like we were making the same call, we thought we were. But due to the default implementation's default values, we were actually calling two totally different methods on the two tests, and that only revealed itself when called indirectly.
SO. Because I know there are existing implementations that rely on just passing things straight through to what was the default implementation,
I've updated that to just be calledUPDATE: I realized it was much cleaner to just remove the default values from the default implementation, so I did that instead.defaultRequestBodyso it can be called from other implementations ofRequestBodyCreator(you can see an example in how I set up the updatedApolloRequestCreator). This also allows you to grab the default and modify it instead of having to redo the whole thing yourself.