Skip to content

Add config option to omit export of empty fragment subtypes #7782

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peternedap
Copy link

@peternedap peternedap commented Apr 21, 2022

Description

Adds an extra config field for omitting empty objects when using inline fragment spreads in combination with skipTypename.
This makes an upgrade from @graphql-codegen/typescript-operations version 1.18.0 to version 2 possible for people who run into problems with this. This also allows the upgrade from graphql 15 to 16, since typescript-operations 1.18 is incompatible with version 16.

Related # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots/Sandbox (if appropriate/relevant):

How Has This Been Tested?

Added unit tests:

  • Config Should not generate empty fragments when omitEmptyFragments is set to true
  • Config Should generate empty fragments when omitEmptyFragments is set to false

Test Environment:

  • OS: Ubuntu
  • @graphql-codegen/...: typescript-operations
  • NodeJS: v17.8.0

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

n/a

@changeset-bot
Copy link

changeset-bot bot commented Apr 21, 2022

⚠️ No Changeset found

Latest commit: 4123c09

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

@vercel
Copy link

vercel bot commented Apr 21, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
graphql-code-generator ✅ Ready (Inspect) Visit Preview Apr 21, 2022 at 7:12AM (UTC)

@peternedap
Copy link
Author

As a first-time contributor I'm not allowed to run the other workflows

Copy link
Collaborator

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

Could you explain why you would want to introduce a flag for generating wrong types? Omitting the types of empty fragments can cause runtime errors as the TypeScript type would no longer mirror the actual GraphQL result type.

@n1ru4l n1ru4l added the waiting-for-answer Waiting for answer from author label May 4, 2022
@peternedap
Copy link
Author

We use global unique IDs in our code base, and every queryable type implements

""" Node, to make a type queryable, it needs to implement this interface. """
interface Node {
    """ A node requires an ID. """
    id: ID!
}

Furthermore, every queryable type is queried via a spread:

""" The query root. """
type Query {
    """ Retrieves a Node through its ID. """
    node(id: ID!): Node
}

type Person implements Node {
    """ The ID of the person. """
    id: ID!

    """ Self explanatory. """
    name: String
}

queried by

export const PERSON_DATA_QUERY = gql`
    query person($id: ID!) {
        node(id: $id ) {
            ... on Person {
                id
                name
            }
        }
    }
`;

This is a flow that was well supported in old versions, but with new versions we run into the problem that the resulting generated type cannot be easily narrowed to only the person data. We'd either have to include the typename and add an extra check to every query result, or find a way to remove {} from the resulting query types.
The resulting type can indeed be mismatched runtime when a different ID than that of a person is given as a query variable, but that would also result in different errors since there'd be no Person with that ID (because of the global unique IDs).

This MR thus introduces an optional way to reintroduce support for the previously existing behaviour where empty spreads were not added to the type.

@vercel
Copy link

vercel bot commented May 10, 2022

@peternedap is attempting to deploy a commit to the The Guild Team on Vercel.

A member of the Team first needs to authorize it.

@OscarBarrett
Copy link

Could you explain why you would want to introduce a flag for generating wrong types? Omitting the types of empty fragments can cause runtime errors as the TypeScript type would no longer mirror the actual GraphQL result type.

Please also see the example in #6873.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-answer Waiting for answer from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants