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

Update documentation about constructors for the v3 migration guide #3268

Merged
merged 1 commit into from
Jul 24, 2021

Conversation

PHPirates
Copy link
Contributor

@PHPirates PHPirates commented Jul 24, 2021

I am using version 3.0.0-alpha02 in a Kotlin project.

I found that the class mentioned in the documentation, DefaultHttpNetworkTransport, did not exist, but after some fiddling around I pieced something together that seems to work. Please correct me if this is not what you intended.

Other than this, the migration to v3 was pretty smooth. Many thanks for the Kotlin support!

Offtopic for this PR:
One other minor confusion was about the @optional directive, I first thought that you wanted to replace nullability by the Optional class just as some other programming languages do, so I replaced all nullable variables in my query by Optional parameters.
But then I found that it didn't work because graphql will still attempt to pass null values for some reason (not in my code, I got it back as an error in the query result so no stacktrace. Maybe on the side of the GitHub api?).
So I keep my query as query MyQuery($nonNullable: String!, $nullable: String). I think the documentation just meant to speak about a situation where you really have optional parameters that are there or are not there (not sure when that would arise).
If I misunderstood the intention, I would appreciate if you let me know.
But I know that my story is pretty vague, so if you can't make sense of it please ignore me :)

@apollo-cla
Copy link

@PHPirates: 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/

@martinbonnin
Copy link
Contributor

martinbonnin commented Jul 24, 2021

Many thanks for the documentation fix 🙏.

About optional and/or nullable GraphQL variables, this is indeed super confusing. The GraphQL type system doesn't have optional types, only nullable. But there are cases where it is important to distinguish. For an example, in input types.

If you're coming from 2.x, the main change to remember is that Input is renamed to Optional.

The other change is that nullable GraphQL variables will be non-optional Kotlin properties by default (yea, I know this is a mouthful...)

For your query:

query TotalIssues($repository: String!, $owner: String!, $issuesCursor: String, $pullRequestsCursor: String, $issues: Int!) {

That should generate something like:

class TotalIssues(
  val repository: String,
  val owner: String,
  val issueCursor: String?, // Note how this is not `Optional<String?>`
  val pullRequestsCursor: String?, // Note how this is not `Optional<String?>`
  val issues: Int
)

If you want to be able to omit issueCursor (in order to use the schema defaultValue), you can declare the variable optional ($issuesCursor: String @optional), which will generate val issuesCursor: Optional<String?>.
The rationale was that in most cases, variables are meant to be present (else they wouldn't be added in the first place)

I hope that makes sense? Or would you have preferred the other logic instead?

@PHPirates
Copy link
Contributor Author

The GraphQL type system doesn't have optional types, only nullable.

Aha, I didn't know that, because of my lack of GraphQL knowledge I was assuming the contrary based on the apollo docs and that's probably what got me confused.

Yes, it is clear now, thanks!

@martinbonnin
Copy link
Contributor

👍👍 that'd certainly be a nice blog post along the way...

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.

3 participants