Create FetchOptions to allow GET Requests like in Web#572
Create FetchOptions to allow GET Requests like in Web#572designatednerd merged 12 commits intoapollographql:masterfrom
Conversation
|
@dmandarino: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
designatednerd
left a comment
designatednerd
left a comment
There was a problem hiding this comment.
This looks fantastic! I had a few comments and suggestions - let me know if you'd prefer I merge and implement them myself or if you'd prefer to make some of these changes. Great work!
|
@designatednerd Thank you for your review! I just did the changes you asked for. |
designatednerd
left a comment
designatednerd
left a comment
There was a problem hiding this comment.
Much better - just a couple minor things you're welcome to either fix or leave to me. Let me know what you'd prefer.
| do { | ||
| request.httpBody = try serializationFormat.serialize(value: body) | ||
| } catch { | ||
| completionHandler(nil, GraphQLHTTPRequestError(kind: .serializedBodyMessageError)) |
There was a problem hiding this comment.
You should probably return here and above to prevent the rest of this from executing
There was a problem hiding this comment.
Hi!
This method send<Operation> should return a Cancellable. In this case, it should return a task after the request. If it fails, it will execute the request and returns another error.
As the task must resume ( task.resume() ), what do you think of instead of do/try, we use a try? to let the request executes and returns the correct error? Another option would be change the return type to be optional as Cancellable?. But I don't like that much this approach because it would change the whole architecture of apollo-client and also, the request would never be executed in this case. And I'm not sure that it would be a good idea for clients.
What do you think? Do you have any suggestion?
There was a problem hiding this comment.
I'll poke around - I think there's probably something else that conforms to Cancelable that we can return here
There was a problem hiding this comment.
Okay! Thank you, once again!
There was a problem hiding this comment.
Right now the only non private object/struct is an extension of URLSessionTask declared inside this class. The others are the classes GraphQLQueryWatcher and WebSocketTask which are not supposed to be used in here.
But I was thinking about it... A request can be executed without a request body or a query parameter. It's like a request to an endpoint without parameters. So, if this try fails, it won't have a request body, and so, when the task resume, it will just return an error and the reason for it.
But also, the only way for this to fail, right now, would be in the case that Operation fails to be constructed. Which means a bad format in query that would fail while compiling or maybe during the processing.
Maybe, that's why it was using try! before. I do not like to force a try, but changing it for try? wouldn't cause a crash. And because we are confident that GraphQLOperation only exists if it succeeded parsing those values in query, the worst case scenario would be a request to an endpoint without parameter.
There was a problem hiding this comment.
But once again, tell me what is your opinion, please! I want to help with it, but if you want to keep this change and find another way to solve it, please, be my guest to fix it.
There was a problem hiding this comment.
I think for this PR we're gonna leave it - I'll make a fix that addresses this in other areas as well in a separate PR.
There was a problem hiding this comment.
Ok, thank you for the review. :) !!
9 participants
#570
To give an option of choose between GET and POST as HTTP Method.
So, requests can be cached by API and user can choose the method suits better for each case. Also, some API only GET.
By exposing
FetchOptionsin fetch method, users will be able to choose the appropriate method every time he use it.Like:
fetch(query: GraphQLQuery(), fetchOptions: .GET)