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

Always add __typename to abstract types #91

Merged
merged 1 commit into from
Nov 8, 2021

Conversation

gmac
Copy link
Contributor

@gmac gmac commented Nov 5, 2021

Preemptively fixing the same issue described in #81, but with __typename...

The (potential) problem

Given that the execution resolver uses __typename while assembling abstract types, we need to assure that a typename is always returned. However, the automated __typename hints are currently only added to user-defined fragments, and those aren't guaranteed to be present when making abstract type selections (case of point: selecting base interface fields).

While I haven't actually made this scenario fail, I strongly suspect that it can be broken.

The (preemptive) fix

This adds __typename to all abstract types. In the event that the user doesn't make a fragment selection on an abstract, the selection will still request typename for resolution purposes.

@gmac gmac requested a review from a team as a code owner November 5, 2021 02:30
@gmac gmac force-pushed the abstract-typename-hints branch from 3d74958 to 2c776bd Compare November 5, 2021 02:40
@@ -2,4 +2,5 @@
.vscode
.history
.idea
.DS_Store
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bane of MacOS developers...

@codecov-commenter
Copy link

Codecov Report

Merging #91 (2c776bd) into main (0813d5d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   67.15%   67.17%   +0.02%     
==========================================
  Files          24       24              
  Lines        2749     2754       +5     
==========================================
+ Hits         1846     1850       +4     
- Misses        748      749       +1     
  Partials      155      155              
Impacted Files Coverage Δ
plan.go 82.40% <100.00%> (+0.35%) ⬆️
auth.go 86.79% <0.00%> (-0.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0813d5d...2c776bd. Read the comment docs.

@gmac
Copy link
Contributor Author

gmac commented Nov 8, 2021

@nmaquet – this is ready for consideration.

Copy link
Contributor

@nmaquet nmaquet left a comment

Choose a reason for hiding this comment

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

Another nice find, thanks @gmac.

@suessflorian
Copy link
Contributor

I'm thinking; if we guarantee a __typename injection when the parent type presents itself as an abstract type, can we then omit the injection when encountering fragment spreads?

@gmac
Copy link
Contributor Author

gmac commented Nov 8, 2021

@suessflorian — I had the same thought… I haven’t dug deep enough into the execution pipeline yet for the answer though. The wildcard would be if typename is somehow used for looking up fragments within the resolver, at which time a fragment on a concrete type (which is totally valid) would still need typename via the fragment. Leaving the status quo seemed safest.

Copy link
Contributor

@nmaquet nmaquet left a comment

Choose a reason for hiding this comment

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

Thanks @gmac!

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