GET as a transport option instead of a parameter + ErrorCancellable by designatednerd · Pull Request #599 · apollographql/apollo-ios

designatednerd · 2019-07-01T09:19:28Z

I had a chat with @martijnwalraven about how using GET vs POST was handled in #572, and he had a couple good points I missed in the review: First, passing the fetch method as a parameter doesn't make sense if it's going to eventually be handed off to a web socket, since it'll ignore that parameter anyway. Second, if someone wants to use GET for a fetch request, they almost certainly want to use it for all fetch requests.

His suggestion (which I agreed with and implement here) was to use a parameter on the HTTPNetworkTransport to set whether fetches should use GET or POST - I left the default as POST to match existing behavior, but I think this will make things a lot simpler for the end user.

@dmandarino I'd love to hear your thoughts on this as it's a pretty significant change to what you proposed, but I think with the same end result.

Also in this PR:

Added ErrorCancelable return type which allows you to return a Cancellable after encountering an error that should early-exit.
Updated .xcodeproj files to ensure 2-space formatting for the Apollo projects (note: Didn't fix any existing 4-space formatting, will do that in a separate PR when I have more time)
Fixed a couple of warnings in the web socket tests
Started pulling a few things out into their own files to make them easier to find

martijnwalraven · 2019-07-01T21:31:36Z

+// MARK: - Early-Exit Helper
+
+/// A class to return when an error that should cause us to bail out of something still needs to return `Cancellable`.
+public final class ErrorCancellable: Cancellable {


I'm wondering if this should really be tied to errors, or whether something more generic like an EmptyCancellable or NullCancellable (from null object pattern), would make sense.

Ooh good call - EmptyCancellable is way more accurate

… need `Cancelable` after a completion handler has been called.

…everywhere; FetchHTTPMethod -> GraphQLHTTPMethod

(These are no longer necessary)

dmandarino

@designatednerd thank you for considering me in this PR and for asking for my opinion.

I just did as parameter, so the user can choose between GET and POST whenever he/she wants to without instantiating Apollo again. And I thought that int the future, persisted query would be a parameter too.

But I checked out on others proposes in issues and open PR, and most of them are suggesting to use it as a property in HTTPNetworkTranspot. So I agree with this solution and that it looks better in this way, since WebSocket was ignoring the HTTPMethod parameter.

And I liked the way you solved the Cancellable issue and return. Nice!

dmandarino · 2019-07-02T13:02:40Z

      } else {
-        completionHandler(nil, GraphQLHTTPRequestError(kind: .serializedQueryParamsMessageError))
+        completionHandler(nil, GraphQLHTTPRequestError.serializedQueryParamsMessageError)
+        return EmptyCancellable()


Nice touch!

SirensOfTitan · 2019-07-02T14:27:29Z

@designatednerd: Perhaps I'm thinking too big picture here, but it might be worthwhile to look how custom extensions will evolve as apollo-ios does:

Will apollo-ios adopt the ApolloLink strategy like other clients?
Or perhaps something more idiomatic swift?

... we've been using NetworkTransports to compose different layers of behavior together into one larger NetworkTransport. For example, we have one NetworkTransport for Authentication-retries (i.e. when the user has to request a token from the server to continue requests), and (as of yesterday) one for PersistedQuery management.

If the GET query option is baked into HTTPNetworkTransport (which does feel a lot cleaner as some transports like WebsocketTransport don't need it) as-is, we'd need to re-create network transports for the composition pattern I mentioned above to work (as we'd first want to optimistically send the GET request then send a POST for the full query if we don't have the query cached).

designatednerd · 2019-07-02T15:02:34Z

@SirensOfTitan Definitely good to keep the big picture in mind but I think at this point, particularly given that a) We're at a 0.x release and will be for a while, so "This may evolve" is kind of baked in, and b) People have been requesting this feature for literally years, I would prefer to err on the side of shipping this and making changes when we make decisions about the ApolloLink strategy later.

dmandarino · 2019-07-02T15:17:31Z

I agree with @designatednerd. People are asking for it since 2017 and there were issues opened last year too. The implementation may change in the future while the project goes on, and the need changes.

SirensOfTitan · 2019-07-02T15:20:20Z

@designatednerd @dmandarino Sorry, I'm not arguing against GET, I'm saying that the general, fuzzy idea of how the API will evolve could act as a useful heuristic as to whether we should keep GET as a parameter in send or as a parameter on HTTPNetworkTransport.

designatednerd · 2019-07-02T15:27:49Z

Totally understand your point @SirensOfTitan, but at this point I want to get this out the door for day to day usage.

I'm definitely planning on revisiting some of this architecture when I get the bugs to a manageable point, but with 173 open issues (as of right this second) that may be a minute 😃

# Conflicts: # Sources/Apollo/ApolloClient.swift # Sources/Apollo/GraphQLQueryWatcher.swift

designatednerd · 2019-07-02T19:05:25Z

^ taking this as an approval and merging!

designatednerd requested a review from martijnwalraven July 1, 2019 09:20

designatednerd mentioned this pull request Jul 1, 2019

Header configuration + Fix GET with null parameter #602

Merged

martijnwalraven reviewed Jul 1, 2019

View reviewed changes

designatednerd added 15 commits July 2, 2019 09:16

Add ErrorCancellable types so we can bail out of things which still…

7c5be67

… need `Cancelable` after a completion handler has been called.

update to 2-space indentation on all projects by default

4b08fa5

Make error with no additional parameters just an enum

e369360

tweak a few descriptions for more consistent swifting

07199a9

pull request and response errors out into their own files

3c97896

make error cancelable final

aae0adb

enable code coverage for main, SQLite, and WebSocket schemes

3d494a0

add public initializer and fix indentation for ErrorCancellable

c6132ca

fix swift warnings in the WebSocket tests

2c6bbc9

Make GET for queries an option on transport rather passing parameter …

ef5d6de

…everywhere; FetchHTTPMethod -> GraphQLHTTPMethod

HTTP method no longer needs to be public

6564150

rm whitespace

b02a623

Separator and explanation of why the init is public

5a759de

remove tests that were duplicated to test GET

2d2853a

(These are no longer necessary)

rename ErrorCancellable -> EmptyCancellable for flexibility

6a89b50

designatednerd force-pushed the update/get-as-option branch from d2059ab to 6a89b50 Compare July 2, 2019 07:17

Fix a couple ErrorCancellables the renamer missed 🤦‍♀️

56037ca

designatednerd mentioned this pull request Jul 2, 2019

Add support for Automated Persisted Queries #601

Closed

2 tasks

dmandarino approved these changes Jul 2, 2019

View reviewed changes

designatednerd mentioned this pull request Jul 2, 2019

Feature: Auto Persisted Queries with CDN Support #608

Closed

designatednerd added 2 commits July 2, 2019 18:14

Merge branch 'master' into update/get-as-option + fix merge conflicts

52c95bd

# Conflicts: # Sources/Apollo/ApolloClient.swift # Sources/Apollo/GraphQLQueryWatcher.swift

fix indentation in web socket transport

94d7bd7

designatednerd merged commit 9451b35 into master Jul 2, 2019

designatednerd deleted the update/get-as-option branch July 2, 2019 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GET as a transport option instead of a parameter + ErrorCancellable #599

GET as a transport option instead of a parameter + ErrorCancellable #599
designatednerd merged 18 commits intomasterfrom
update/get-as-option

designatednerd commented Jul 1, 2019 •

edited

Loading

Uh oh!

martijnwalraven Jul 1, 2019

Uh oh!

designatednerd Jul 2, 2019

Uh oh!

dmandarino left a comment

Uh oh!

dmandarino Jul 2, 2019

Uh oh!

SirensOfTitan commented Jul 2, 2019

Uh oh!

designatednerd commented Jul 2, 2019

Uh oh!

dmandarino commented Jul 2, 2019

Uh oh!

SirensOfTitan commented Jul 2, 2019

Uh oh!

designatednerd commented Jul 2, 2019

Uh oh!

designatednerd commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

designatednerd commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnwalraven Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

designatednerd Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

dmandarino left a comment

Choose a reason for hiding this comment

Uh oh!

dmandarino Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

SirensOfTitan commented Jul 2, 2019

Uh oh!

designatednerd commented Jul 2, 2019

Uh oh!

dmandarino commented Jul 2, 2019

Uh oh!

SirensOfTitan commented Jul 2, 2019

Uh oh!

designatednerd commented Jul 2, 2019

Uh oh!

designatednerd commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

designatednerd commented Jul 1, 2019 •

edited

Loading