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

fix: Fix spreading abstract fragments into abstract selections widening the result type #102

Merged
merged 5 commits into from
Mar 2, 2024

Conversation

kitten
Copy link
Member

@kitten kitten commented Mar 2, 2024

Resolves #100

Summary

This improves the type inference for two cases specifically.

When spreading a fragment of an interface (potentially itself) into an interface selection, the type wasn't narrowed even if we had the fragment available and were doing a submatch. This lead to poor type matches when a user wasn't specifying __typename above and below the spread although this isn't necessary, since we have all necessary type information.
We can generically solve this by keeping the PossibleType narrow, specifically by skipping getSelection and using getPossibleTypeSelectionRec directly.

Specifically, when using an unmasked fragment document and repeating the above a similar issue would occur. In such a case the type checker would be forced to accept two unions of two types that aren't mergeable.

There isn't a great solution for this without refactoring a whole lot and making output types less readable, while hurting performance in the process.
There is however a simple solution.

We now always attach a __typename?: PossibleType optional property to possible type selections. This attaches a type to match against for the type checker to merge all possible types’ selections.
While not the perfect solution, this shouldn't have any negative consequences.

  • The property is optional, so unlikely to be freely used without being requested
  • The property may explicitly become never when a branch is missing a typename providing a visual indicator that __typename isn't selected in all cases
  • The property is already likely to be requested, and even if it's aliased, this should work correctly
  • __typename is unlikely to be aliased (which we can also lint for), since it's a system field and unique in behaviour

Set of changes

  • Skip getSelection and use getPossibleTypeSelectionRec directly, where possible
    • Provide the narrowed PossibleType in nested fragment selections
  • Add a __typename?: PossibleType property to possible type selections
  • Add tests for these cases

@kitten kitten requested a review from JoviDeCroock March 2, 2024 22:55
Copy link

changeset-bot bot commented Mar 2, 2024

🦋 Changeset detected

Latest commit: f18b791

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

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

@kitten kitten changed the title Fix/spreading subtypes into type fix: Fix cases where spreading abstract fragments into abstract selections would widen the type Mar 2, 2024
@kitten kitten changed the title fix: Fix cases where spreading abstract fragments into abstract selections would widen the type fix: Fix spreading abstract fragments into abstract selections widening the result type Mar 2, 2024
@kitten kitten merged commit d279bf4 into main Mar 2, 2024
1 check passed
@kitten kitten deleted the fix/spreading-subtypes-into-type branch March 2, 2024 22:59
@github-actions github-actions bot mentioned this pull request Mar 2, 2024
@kitten kitten added this to the Are We Fast Yet? milestone Mar 22, 2024
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.

__typename not properly disambiguating the union when it's included in a fragment
1 participant