Skip to content
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

feature(defer): Partial and Incremental Parsing #217

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Dec 21, 2023

Closes apollographql/apollo-ios#3277
Closes apollographql/apollo-ios#3145
Closes apollographql/apollo-ios#3147

To recap the progress so far:

  • main has some defer changes merged for codegen but they are disabled
  • feature/defer-execution-networking is where I'm collecting the rest of the changes that will enable preview releases.

This PR brings parsing of the initial response (partial) and subsequent responses (incremental) to the defer implementation. The changeset includes changes to the templates and rendering but mostly are focused on the execution and networking layers. Details below:

  • Removed hasDeferredFragments from the Operation definition. This was only used for the request HTTP headers but that behaviour has changed too and operations now get a deferredFragments property to help with incremental execution.
  • Deferrable conformance was moved to the InlineFragment declaration to match the definition of Fragment. This allows us to clean up some of the inline fragment code generation.
  • IncrementalJSONResponseParsingInterceptor is a new interceptor built to handle incremental payloads and combining them with previously parsed results.
  • There is no collection of deferred fields when a .deferred selection set is encountered because those fields will not be present in the response data, but the deferred fragment is marked for future collection. Incremental execution will then pick up the deferred fragments fields for collection when the relevant response is received.
  • GraphQLExecutor now has two entry points for execution; one used for the initial partial response and the other used during incremental execution parsing. They both converge in subsequent execution calls but the difference is in the generic constraints of the function and which selection set they operate on - RootSelectionSet or SelectionSet.
  • GraphQLResponse has been broken up into a base struct named AnyGraphQLResponse which provides common logic between GraphQLResponse, used for partial responses, and IncrementalGraphQLResponse which is used for incremental response parsing.
  • DataDictMapper was broken out of GraphQLSelectionSetMapper and handles the conversion of JSON data into the DataDict format.
  • GraphQLResult now has the ability to merge in data from an IncrementalGraphQLResult. This is used during incremental response parsing and is required because of the way the interceptors require a single 'complete' result to be passed into the call to proceedAsync. The partial and incremental results are built upon each other during each incremental response storing the 'completed' response in the incremental interceptor which is unique to each operation request.
  • PathEntry from GraphQLError was broken out into it's own type for reuse in the incremental execution.
  • Caching is disabled for now so the cache write interceptor simply ignores caching behaviour for any operation using @defer. This will come in a later PR.

There are no tests included with this PR yet, they're still coming. Last of the tests outstanding is for incremental execution.

Copy link

netlify bot commented Dec 21, 2023

Deploy Preview for eclectic-pie-88a2ba canceled.

Name Link
🔨 Latest commit a6fc75a
🔍 Latest deploy log https://app.netlify.com/sites/eclectic-pie-88a2ba/deploys/6584c0e4b3bf36000887d7fa

@calvincestari calvincestari changed the title feature: Defer incremental parsing feature(defer): Partial and Incremental Parsing Dec 21, 2023
@calvincestari calvincestari force-pushed the defer/partial-and-incremental-execution branch from 712589c to 9d54318 Compare December 21, 2023 22:36
@calvincestari calvincestari changed the base branch from main to feature/defer-execution-networking December 21, 2023 23:38
apollo-ios/Sources/Apollo/GraphQLExecutor.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/Apollo/GraphQLResult.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/Apollo/GraphQLResult.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/ApolloAPI/GraphQLOperation.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/ApolloAPI/GraphQLOperation.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/Apollo/IncrementalGraphQLResponse.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/Apollo/IncrementalGraphQLResponse.swift Outdated Show resolved Hide resolved
@calvincestari
Copy link
Member Author

@AnthonyMDev I think this PR is finally ready for review.

@calvincestari calvincestari marked this pull request as ready for review March 16, 2024 00:29
@calvincestari calvincestari requested a review from a team as a code owner March 16, 2024 00:29
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just reviewed the production code, and had a few very minor comments. I'm going to review the tests shortly as well!

)
}

func execute<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we could really still just use the one execution function above if we just change it from needed RootSelectionSet to just SelectionSet. It doesn't look like there is any other actual deviation in functionality between these two functions. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I've made a note that we should chat about it this week and if there are changes I'll make a separate PR for that.

apollo-ios/Sources/Apollo/GraphQLResult.swift Outdated Show resolved Hide resolved
apollo-ios/Sources/ApolloAPI/GraphQLOperation.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests LGTM! Just a couple minor questions.

@calvincestari calvincestari merged commit 9e7af06 into feature/defer-execution-networking Mar 19, 2024
16 checks passed
@calvincestari calvincestari deleted the defer/partial-and-incremental-execution branch March 19, 2024 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants