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

Tutorial - Update Link.comments return type #3545

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Conversation

Urigo
Copy link
Collaborator

@Urigo Urigo commented Dec 12, 2024

I was thinking about this again and went back to the original type :)

But it was after I learned a lot from @n1ru4l and @eddeee888 and I've added many more comments about the comments return types and why we chose this, so I think overall it was worth the long trip...

Copy link

changeset-bot bot commented Dec 12, 2024

⚠️ No Changeset found

Latest commit: d258f21

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 12, 2024

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🟢
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

Copy link
Contributor

github-actions bot commented Dec 12, 2024

💻 Website Preview

The latest changes are available as preview in: https://979f63a9.graphql-yoga.pages.dev

Copy link
Contributor

github-actions bot commented Dec 12, 2024

✅ Benchmark Results

     ✓ no_errors{mode:graphql}
     ✓ expected_result{mode:graphql}
     ✓ no_errors{mode:graphql-jit}
     ✓ expected_result{mode:graphql-jit}
     ✓ no_errors{mode:graphql-response-cache}
     ✓ expected_result{mode:graphql-response-cache}
     ✓ no_errors{mode:graphql-no-parse-validate-cache}
     ✓ expected_result{mode:graphql-no-parse-validate-cache}
     ✓ no_errors{mode:uws}
     ✓ expected_result{mode:uws}

     checks.......................................: 100.00% ✓ 511150      ✗ 0     
     data_received................................: 2.1 GB  14 MB/s
     data_sent....................................: 103 MB  685 kB/s
     http_req_blocked.............................: avg=1.59µs   min=992ns    med=1.37µs   max=2.01ms   p(90)=2.06µs   p(95)=2.26µs  
     http_req_connecting..........................: avg=9ns      min=0s       med=0s       max=1.88ms   p(90)=0s       p(95)=0s      
     http_req_duration............................: avg=366.38µs min=223.15µs med=332.92µs max=20.77ms  p(90)=473.25µs p(95)=495.56µs
       { expected_response:true }.................: avg=366.38µs min=223.15µs med=332.92µs max=20.77ms  p(90)=473.25µs p(95)=495.56µs
     ✓ { mode:graphql-jit }.......................: avg=293.68µs min=223.15µs med=276.69µs max=20.77ms  p(90)=308.86µs p(95)=326.14µs
     ✓ { mode:graphql-no-parse-validate-cache }...: avg=498.52µs min=411.57µs med=473.46µs max=6.39ms   p(90)=514.85µs p(95)=552.33µs
     ✓ { mode:graphql-response-cache }............: avg=349.96µs min=266.48µs med=331.87µs max=7.79ms   p(90)=362.8µs  p(95)=374.57µs
     ✓ { mode:graphql }...........................: avg=372.8µs  min=281.77µs med=341.27µs max=14.75ms  p(90)=408.48µs p(95)=465.65µs
     ✓ { mode:uws }...............................: avg=353.04µs min=275.38µs med=332.83µs max=6.46ms   p(90)=371.39µs p(95)=397.35µs
     http_req_failed..............................: 0.00%   ✓ 0           ✗ 255575
     http_req_receiving...........................: avg=33.42µs  min=16.38µs  med=32.86µs  max=3.09ms   p(90)=39.8µs   p(95)=42.68µs 
     http_req_sending.............................: avg=8.83µs   min=6.06µs   med=7.78µs   max=359.21µs p(90)=11.23µs  p(95)=12.52µs 
     http_req_tls_handshaking.....................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting.............................: avg=324.13µs min=191.13µs med=291.58µs max=20.62ms  p(90)=430.42µs p(95)=451.22µs
     http_reqs....................................: 255575  1703.817881/s
     iteration_duration...........................: avg=582µs    min=395.05µs med=544.14µs max=23.3ms   p(90)=692.99µs p(95)=720.44µs
     iterations...................................: 255575  1703.817881/s
     vus..........................................: 1       min=1         max=1   
     vus_max......................................: 2       min=2         max=2   

case, when they will generate the types on their end.
1. `comments: [Comment]`
2. `comments: [Comment!]`
3. `comments: [Comment!]!`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We even have a fourth option: comments: [Comment]!. I don't know if it's worth explaining this one too ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what would you write about that?
can you add a text similar to that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Option 4, [Comment]!, shares semantics with option 1 and 3. Like 1 the array elements can be null or Comment, but like 3, the field itself will always be an array, never null. This type formulation is rarely used because simply omitting a value from the array is typically sufficient to indicate its non-valueless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would maybe alter the documentation here to talk about two aspects which cut across all four combinations:

  1. nullable array elements
  2. nullable array

After talking about those two aspects, one can simply reference which aspects apply to which combinations.

Then, a final word about which formulation is the most popular and why ([X!]! because it provides the simplest way to sufficiently model empty states blah blah).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

shit I missed your comment!
can you create a short PR just with some words?
I can then take the time to turn it into something readable

Urigo and others added 2 commits December 12, 2024 13:34
@Urigo Urigo merged commit c367684 into main Dec 12, 2024
25 checks passed
@Urigo Urigo deleted the tutorial-update-comments-again branch December 12, 2024 12:38
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