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

Add mergeExtensions and toFullSchemaGQLDocument #5162

Merged
merged 6 commits into from
Aug 17, 2023
Merged

Conversation

martinbonnin
Copy link
Contributor

This pull request unfortunately contains a bunch of both binary and source breaking changes. Since apollo-ast was not advertised as much as apollo-api and apollo-runtime, I'm hoping this is OK. I took this opportunity to tweak a bit the public API:

  • okio is not part of stable the public API anymore (it still is experimentally available though)
  • It's not possible to go from String/File to Schema directly anymore, you have to go through GQLDocument:
// Before
Buffer().writeUtf8("type Query { foo: Int }").toSchema()

// After
"type Query { foo: Int }".toGQLDocument().toSchema()

More comments inline

@martinbonnin martinbonnin requested review from a team and BoD as code owners August 7, 2023 10:44
@netlify
Copy link

netlify bot commented Aug 7, 2023

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit a7f442f
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/64ddef3ae68bfd00088a88af

Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

👍

@martinbonnin martinbonnin marked this pull request as draft August 9, 2023 10:51
add GQLDocument.toFullSchemaGQLDocument and GQLDocument.mergeExtensions
@martinbonnin martinbonnin marked this pull request as ready for review August 16, 2023 21:54
@martinbonnin
Copy link
Contributor Author

martinbonnin commented Aug 16, 2023

I reshuffled the introspection bits again into 2 pieces:

  • introspection_reader.kt contains a lenient model of introspection and tries to make a best effort guess when some fields are absent. When recoverable (description missing, directive defintions missing, etc...) a log is printed. When unrecoverable (field definitions, arguments, enumValues, etc...) an error is thrown
  • introspection_writer.kt contains a strict model of the current introspection schema we support.

It's a bit verbose but that duplication makes it easier to handle the edge cases of different servers having different version of introspection

Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

👍

* type Query {
* foo: Int @deprecated(reason: null)
* }
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we handle this case by passing isDeprecated + deprecationReason?

Copy link
Contributor Author

@martinbonnin martinbonnin Aug 17, 2023

Choose a reason for hiding this comment

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

The whole IR is based on the premise that isDeprecated = true implies deprecationReason != null. Kotlin @Deprecated annotation also requires a message. I'm still unclear what the use case is for @deprecated(reason: null). I'll add a comment there

@martinbonnin martinbonnin merged commit 1b411f5 into main Aug 17, 2023
@martinbonnin martinbonnin deleted the introspection branch August 17, 2023 11:29
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.

apollo-ast: support merging definitions without validation
2 participants