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.
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
Create FetchOptions to allow GET Requests like in Web #572
Create FetchOptions to allow GET Requests like in Web #572
Changes from all commits
8cb9fb4
53f2e51
bea8242
ad29887
fcb9533
ef686cb
5ff93a7
dee434b
2a77206
9766548
4f92bee
4a97304
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You should probably return here and above to prevent the rest of this from executing
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.
Hi!
This method
send<Operation>
should return aCancellable
. 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 ofdo/try
, we use atry?
to let the request executes and returns the correct error? Another option would be change the return type to be optional asCancellable?
. But I don't like that much this approach because it would change the whole architecture ofapollo-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll poke around - I think there's probably something else that conforms to
Cancelable
that we can return hereThere 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.
Okay! Thank you, once again!
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.
Right now the only non private object/struct is an extension of
URLSessionTask
declared inside this class. The others are the classesGraphQLQueryWatcher
andWebSocketTask
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 fortry?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you for the review. :) !!