Skip to content

Swift Codegen: Split up appending of fragments to query document#2198

Merged
designatednerd merged 3 commits intoapollographql:masterfrom
andybest:query-document-type-check
Jan 11, 2021
Merged

Swift Codegen: Split up appending of fragments to query document#2198
designatednerd merged 3 commits intoapollographql:masterfrom
andybest:query-document-type-check

Conversation

@andybest
Copy link
Copy Markdown

@andybest andybest commented Jan 6, 2021

Splits the appending of fragments to the query document into separate
sub expressions to fix an issue where a query that references lots of fragments
causes the Swift type checker to fail with the following message:

error: the compiler is unable to type-check this expression in reasonable time; try breaking up the expression into distinct sub-expressions

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Splits the appending of fragments to the query document into separate
sub expressions to fix an issue where a query with lots of fragments
caused the Swift type checker to fail.
@apollo-cla
Copy link
Copy Markdown

@andybest: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link
Copy Markdown
Contributor

@martijnwalraven martijnwalraven left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, this looks good to me. I wish we had tests that referenced more than one fragment, but since we're replacing codegen it may not be worth investing in that.

Copy link
Copy Markdown
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Apologies for the delay - I just tested the artifact from this against iOS. It does make some small changes in terms of how we concatenate fragment definitions, but I believe those should have a negligible impact on performance. I'll wait to merge this until Monday for "Seems like a bad weekend to deploy" reasons and then I'll cut a new version, add the new version to the iOS SDK, and cut a new version of that. Whee!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants