client-preset 4.4.0 contains breaking change; config for it is undocumented & cannot be reverted

[bmulholland]

Which packages are impacted by your issue?

@graphql-codegen/typescript, @graphql-codegen/client-preset

Describe the bug

#10073 made a change that removes types that are often actually used. It was released as a new default in a minor version, despite being a breaking change. On top of that, there's no documentation about it, and the config to revert it appears not to even work. It took me 30 minutes to track down why my mock data generation was broken by this release. Other people have commented on the PR that this is broken for them, but that has been missed so far and hasn't been logged as an issue. I'm surfacing the regression here.

Expected behavior

1. No breaking changes in minor releases
2. Breaking changes clearly documented
3. Config to revert behaviour tested and working

[baraknaveh]

Experiencing the same issue

[recurser]

the config to revert it appears not to even work

Same experience here - I can't figure out how to reverse this default config, and I've had to roll back to the previous version.

[mehdiSlam]

same issue, a lot of types are removed ( used in other types ). It works when i fix the version of the preset to the 4.3.3

[gzm0]

FWIW: We have been broken by this because we (ab-)¹used these types in our unit tests to typecheck fake query results for MockedProvider (of apollo-client).

We could not simply use DocumentType<typeof MY_QUERY> because the query contains fragments. Instead, we resorted to type-hackery that resolves the fragments again (see below).

All-in all, this means we get much better typechecking (fields the query requests are required, fields the query doesn't request are disallowed, etc.). I'm posting the hackery here for consideration and because it might be useful to others.

Fragment Resolution Type-Hackery

import type { DocumentType } from "./gql/index.js";

// Type hackery to resolve fragments in GET_ACTIVE_TODOS.
//
// graphql-codegen goes at lengths to hide fragment fields in the types to
// not introduce undeclared dependencies on fields (that's a good thing).
//
// However, when providing fake data to the queries in the MockedProvider, we need
// unfragmented queries (likely, the client handles that entirely, which makes sense).
//
// It seems there is no utility type to do this out there, so we write our own here.

// Helper: Resolve a fragment at the top-level of the provided type (if it exists).
type ResolveDirectFragments<T> = T extends { " $fragmentRefs"?: object }
  ? FragmentRefTypes<Required<T>[" $fragmentRefs"]> & T
  : T;

// Helper: Resolves fragments on indexed values of `T`.
type ResolveInnerFragments<T> = { [K in keyof T]: ResolveFragments<T[K]> };

// Resolve fragments in the given document type (this is the entrypoint).
//
// Note that fragments can recursively define fragments, so we must resolve
// direct fragments first before we recurse.
type ResolveFragments<T> = ResolveInnerFragments<ResolveDirectFragments<T>>;

// Helper: Converts a ` $fragmentRefs` spec into its document type.
//
// For example: `{ Frag1: Frag1Type, Frag2: Frag2Type }` => `Frag1Type & Frag2Type`
type FragmentRefTypes<T> = UnionToIntersection<T[keyof T]>;

// Helper: Converts a union type to an intersection type.
//
// For example: `x | y | z` => `x & y & z`
//
// To achieve this, we (ab-)use contravariance of function types w.r.t. their arguments.
type UnionToIntersection<U> = ((x: U) => void) extends (x: infer I) => void
  ? I
  : never;

// How to use it

type MyQueryData = ResolveFragments<DocumentType<typeof MY_QUERY>>;

Just to be extra clear: I do not consider $fragmentRefs to be part of the public interface of the client-preset, so this might break any time. However, it feels to me the pattern overall is very useful, so maybe it is feasible to "do it properly" with some support from client-preset.

Footnotes

1. Let the flame war begin 😈 ↩

[bmulholland]

Well, FWIW, this change also breaks https://github.com/ardeois/graphql-codegen-typescript-mock-data (ardeois/graphql-codegen-typescript-mock-data#173), so you're not alone @gzm0. I suspect it's pretty common to need some of these "internals" for testing, especially unit testing, where you might intentionally be working at a lower level than "real" application code.

[nebbles]

PR #10073 that introduced this change configured onlyEnumTypes and onlyOperationTypes to be true under-the-hood (not a great tactic!).

Incidentally, I've been trying to get onlyOperationTypes available as part of the client-preset, following discussion in #8693, as I do want to strip out these additional types from the generated output.

PR #10155 addresses this by making it a preset config option (hopefully merged soon). This could provide people a route to disabling it — by explicitly setting false to get the old behaviour — or #10073 could be reverted to restore old behaviour and allow people like me to opt-in via config (which I think is the right route).

[joulev]

A minor release shouldn’t contain breaking changes at all. Upgrading from 4.3 to 4.4 shouldn’t require code change.

This feature might be good, but it must be opt in, not opt out, in 4.4. I shouldn’t have to explicitly set anything to false – it should still work.

Explicit opt out belongs to v5. I’m not against breaking changes but they should absolutely only take place in major releases.

[ian-twana]

Got caught out by this one as well! Tried to unravel #10073 and set onlyOperationTypes to false and adding the typescript plugin, but then it generated duplicate types.

The solution was to roll back to 4.3.3 looking forward to a fix.

[IsaacInsoll]

I've been caught out by this as well on my project 😐

[eddeee888]

Hi all,

Could you please help me understand how you are using these types?

AFAIK, client preset is not using these types, so we released it as minor. But if client preset is using this type, and they are removed, it should have been a major version bump as we all said here.

[IsaacInsoll]

I'm importing the types into certain typescript files. Perhaps originally choosing the client preset was the wrong option even though it previously worked.

What would be the best preset to use if we want similar functionality to how 4.3 and earlier did it?

[eddeee888]

Maybe a workaround right now is to generate types for non-client preset use cases? e.g.

import type { CodegenConfig } from '@graphql-codegen/cli'
 
const config: CodegenConfig = {
  schema: '/*...*/ ',
  documents: [/*...*/ ],
  generates: {
    './src/graphql/': {
      preset: 'client',
      config: { documentMode: 'string' }
    },
    './src/types.generated.ts': {  // This should generate all the types including the ones no longer in client preset
      plugins: ['typescript'] 
    }
  }
}
 
export default config

[jacquesg]

Maybe a workaround right now is to generate types for non-client preset use cases? e.g.

import type { CodegenConfig } from '@graphql-codegen/cli'
 
const config: CodegenConfig = {
  schema: '/*...*/ ',
  documents: [/*...*/ ],
  generates: {
    './src/graphql/': {
      preset: 'client',
      config: { documentMode: 'string' }
    },
    './src/types.generated.ts': {  // This should generate all the types including the ones no longer in client preset
      plugins: ['typescript'] 
    }
  }
}
 
export default config

The best solution is revert the change, document the breaking change and release it as a new major. From what I can tell, this suggest workaround will still require additional code changes, it is not as simple as just updating the configuration.

[bmulholland]

@eddeee888 As I mentioned above, these types are used for mocking data for tests. There's a mock data lib, listed on the codegen website, at https://github.com/ardeois/graphql-codegen-typescript-mock-data. It generates code that imports the removed types, and is thus entirely broken by this change.

[eddeee888]

Hi @jacquesg,

You are right, you'd need to update your code to use the new generated file for your other use cases.

Client preset doesn't use these types, and the documentation doesn't recommend using them. Unfortunately, there's always a bit of a risk using internal types like these, as they can be removed at any time.

From the perspective of client preset, if this change breaks the way it works, then we would release a new major version. But since this only seems to break other non-supported use cases, we think a minor version justifies it.

---

Hi @bmulholland ,

I believe you can generate a new file, then point the new generated file to it using that plugin's typesFile:

typesFile: '../path/to/new/types.generated.ts'`

[jacquesg]

Hi @jacquesg,

You are right, you'd need to update your code to use the new generated file for your other use cases.

Client preset doesn't use these types, and the documentation doesn't recommend using them. Unfortunately, there's always a bit of a risk using internal types like these, as they can be removed at any time.

From the perspective of client preset, if this change breaks the way it works, then we would release a new major version. But since this only seems to break other non-supported use cases, we think a minor version justifies it.

Hi @bmulholland ,

I believe you can generate a new file, then point the new generated file to it using that plugin's typesFile:

typesFile: '../path/to/new/types.generated.ts'`

If they were internal types, they should never have been annotated with export. Because they were, they become part of the public interface.

[jacquesg]

The workaround is simply not workable. I would need to manually update around 80 files, which is really not okay for a minor version bump.

[jacquesg]

Put differently, the responsible action here is like I suggested, to revert the change and bump the version in accordance with the semver spec. If you don't do that, people, including myself will lose all confidence in what the version number means.

[bmulholland]

@eddeee888 Yeah, to echo what the others are saying: I could have figured out what you're suggesting there on my own... if the change was clearly communicated. Both in terms of the versioning, and what's written down (in changelogs and elsewhere), it wasn't.

You've solved part of my problem, for me, but others will have to do the same investigation and fixes, which is why comms are so important here. Also, as you're calling out, I'm going to have to make substantial changes to my GraphQL config to accommodate this change -- which isn't what I'd expect to have to do for a point upgrade.

Additionally, the configuration to revert the change doesn't work, which is IMO a critical bug with the PR in question. That on its own would be reason enough to either release a hotfix or revert the change.

(I want to note that I really appreciate all the work on this project, and recognize that the change in question is in fact a good one! It's great stuff. It was just released and communicated in a way that is causing others to spend a lot of time and frustrating upgrading.)

[LucidityDesign]

same issue, a lot of types are removed ( used in other types ). It works when i fix the version of the preset to the 4.3.3

I had to downgrade @graphql-codegen/cli to 4.0.1, too.

[ethanmick]

This caused breaking changes within our app. Minor releases causing such breaking changes is not expected, could this please be reverted?

[eddeee888]

Hi all,

We have decided to roll forward with this change.

The main reason we don't roll back is because it is going to impact users who don't use these internal types. These types are considered internals because they not used by client preset, and using them is neither recommended nor documented. These types are indeed exported, but it is not our intention for them to be used for non client preset purposes.

However, we understand that this change impacted some in the community, so we are enabling the option to pass through both onlyOperationTypes and onlyEnumTypes in #10178. Note that this is only a temporary measure, and these options will be removed in future major versions.

I believe this is a good compromise to ensure we don't impact the others in the community, whilst allowing enough time for everyone to migrate over.

---

This is available in the following alpha version. If this works, I'll get it released ASAP.

@graphql-codegen/client-preset@4.5.0-alpha-20241021142505-3cc3857b9f672a25cb80ce71439c9098f30fc013.

Note that there is a warning against using these options:

Here's how you can use it:

{
  generates: {
    "src/graphql": {
      preset: 'client',
      config: {
        onlyEnumTypes: false,
        onlyOperationTypes: false,
      }
    }
  }
}

In future versions, it is recommend that you generate a separate file for other purposes:

{
  generates: {
    "src/graphql": {
      preset: 'client',
      config: { /* `onlyEnumTypes` and `onlyOperationTypes` no longer work */ }
    },
    "src/graphql/types.generated.ts": { // Use this file for non client preset purposes
      plugins: ['typescript']
    }
  }
}

[jacquesg]

@eddeee888 Thank you for your effort to try and make this more palatable for people, but the issue remains that it is still a breaking change, and the defaults are the wrong way round. Users should not be required to make changes with a minor bump.

Like I eluded to previously, this makes the version numbering scheme pretty much useless.

[zackmckennarunpod]

So, for some context as to why people might be misusing these types, the first time I installed codegen, I did it after following a tutorial here and here where these generated types are referenced and used.

import type { Track } from '../__generated__/graphql'

I could not understand why the types were missing. Since this was not working, I added the typescript plugin, and ran an afterAllWriteFile script to add the types to my generated types file. I now understand that the tutorial was incorrectly using the types. What is the intentional way of doing this, instead of using these types when creating your components? If the types were not expected to be used, or what is best practice so I can properly configure my codegen setup and usage in my application?

[nlarsson]

I think I need some help understanding "... going to impact users who don't use these internal types". Before 4.4.0 these internal types was generated regardless? How can these users be affected more than those codebases that broke because of a minor release?

[Dallas62]

Hi @eddeee888,

Would be better to rollback in hotfix, and set back your target change in major/feature release with migration guide.

Semver is not about used or not, but public API. Even If it was set as public by error.

[jacquesg]

I think I need some help understanding "... going to impact users who don't use these internal types". Before 4.4.0 these internal types was generated regardless? How can these users be affected more than those codebases that broke because of a minor release?

This is exactly it. No one could have relied on the new functionality until 4.4.0 was released, whereas, there are clearly loads of people that were relying on the documented or not, functionality provided before 4.4.0.

This is clearly painful for a number of people, which makes it obvious (to me at least) that it is unreasonable to expect everyone to go and change their code without any forewarning.

This is exactly what semver is supposed to give you. Confidence in the semantics of a version.

[irisgrunn]

The main reason we don't roll back is because it is going to impact users who don't use these internal types. These types are considered internals because they not used by client preset, and using them is neither recommended nor documented. These types are indeed exported, but it is not our intention for them to be used for non client preset purposes.

This is a joke, right? They became part of the api when they were exported. So it's still a breaking change which according to semver should have been a new major version. Minor versions and patches should always be backwards compatible.

I also don't see how rolling back these changes will impact people who don't use these types? They'll be where they were a few weeks ago with everything still functional. You could even extend the api to make this new behavior an option, as long as it remains backwards compatible.

The best course of action would be to rollback this change in a patch (4.4.1) and release this new behavior in a new major version (5.0.0)

[yykcool]

I believe this is a good compromise to ensure we don't impact the others in the community, whilst allowing enough time for everyone to migrate over.

with the rollback:

• changes now respect semver standards, all is right in the world :)
• people who don't need the types, now get the types back
• codegens that were silently and mysteriously breaking stops breaking

with no rollback:

• minor bump silently introduces a breaking change not captured by docs / changelogs / search engine webcrawlers....making it brain-meltingly irritating to debug because everyone's first response would be "packages look normal, this probably broke because of something i / we did" and traipse off on a grand adventure chasing irrelevant leads to ultimately realise that it was in fact not their fault and some package's version number lied to them
• people who don't need the types get a smaller bundle size
• developers whose codegen pipeline is now breaking has to waste hours eliminating the possibility that it was their mistake before stumbling on this thread

With >1.7m weekly downloads, you'd be looking at ~1m wasted man hours assuming this hits just 5% of this month's install base alone , assuming a very confident estimate of each codebase only taking 3 man hours to uncover this issue (it took me ~5).

Also, odds are high that if people haven't already hooked up tree-shaking, they probably aren't bothered by the increase in bundle size from the extra types lying around.

Dunno about you, but rolling back sounds like a way better compromise imo

[sporto]

Please rollback this change, this breaking change is not fine in a minor version.
Make a major version of this change, is not a big deal making a major.

[jfriestedt]

This cost our team half a day trying to debug and address and will doubtless continue to affect others the longer it is not reverted in a new release.

As others have said, regardless of whether the internal types were meant to be exposed and used, they were, and to remove them is a breaking change that should be done in a major version release with a migration guide. Please revert this optimization in a patch release.

[ohanslik-vendavo]

The reason why we use those "internal" types in our project is simple. Operations inlines most of the types so if the internal type is:

export type DecimalCell = {
  __typename?: 'DecimalCell';
  decimalValue?: Maybe<Scalars['Decimal']['output']>;
};

then operation contains inlined

{ __typename: 'DecimalCell', decimalValue?: number | null }

We haven't been able to change that via config, therefore we rather use the named "internal" type.

[joshbuddy]

Respectfully, it doesn't inspire a lot of confidence in this project if we can't rollback from this and then introduce this change in a way that allows people to upgrade gracefully. Whether you intended for these types to be public is beside the point. By exporting them you made them part of your interface, and while I can see you never intended for this to be a public, practicing good release hygiene shows people they can depend on this project.

[yykcool]

For folks still affected, i've managed to hack out a not very elegant workaround that does not involve manually refactoring every single affected import ... statement.

This hack involves making graphql-codegen work with 1 instance of the client preset, and 1 extra instance of the typescript plugin, with a cleanup script to remove types that appear more than once in the output ts file

codegen.ts:

import { CodegenConfig } from "@graphql-codegen/cli";
import { loadEnvConfig } from "@next/env";

loadEnvConfig(process.cwd());

const gqlUrl = ...;
const apiKey = ...;

const config: CodegenConfig = {
  schema: {
    [gqlUrl]: {
      headers: { apiKey }
    }
  } as CodegenConfig["schema"],
  documents: ["pages/**/*", "src/**/*", "components/**/*"],
  hooks:{
    afterAllFileWrite:['yarn tsx scripts/removeDuplicateTypes.ts']
  },
  generates: {
    //enable if you need to inspect the schema for debugging
    // "./gql/schema.graphql": {
    //   plugins: ["schema-ast"],
    //   config: {
    //     includeDirectives: true
    //   }
    // },
    "./gql/": {
      preset: "client",
      plugins: ["typescript"],
      presetConfig: {
        gqlTagName: "gql",
        strictScalars: true,
        onlyEnumTypes: false,
        onlyOperationTypes: false
      },
      config: {
        scalars: {
          UUID: "string",
          Datetime: "string",
          Date: "string",
          BigInt: "string"
        }
      }
    }
  },
  ignoreNoDocuments: true
};

export default config;

<working_directory>/scripts/removeDuplicateTypes.ts

import * as fs from "fs";
import * as path from "path";
import * as ts from "typescript";

function removeDuplicateTypes(filePath: string): void {
  // Read the file
  const fileContent = fs.readFileSync(filePath, "utf-8");

  // Create a source file
  const sourceFile = ts.createSourceFile(
    filePath,
    fileContent,
    ts.ScriptTarget.Latest,
    true
  );

  // Create a map to store unique type definitions
  const uniqueTypes = new Map<string, ts.Node>();

  const transformerFactory: ts.TransformerFactory<ts.SourceFile> =
    (context) => (sourceFile) => {
      // Function to process nodes
      function visit(node: ts.Node): ts.Node | undefined {
        // Check if the node is a type alias declaration, interface declaration, or enum declaration
        if (
          ts.isTypeAliasDeclaration(node) ||
          ts.isInterfaceDeclaration(node) ||
          ts.isEnumDeclaration(node)
        ) {
          const name = node.name.text;
          if (!uniqueTypes.has(name)) {
            uniqueTypes.set(name, node);
            return node;
          }
          return undefined;
        }

        return ts.visitEachChild(node, visit, context);
      }
      return ts.visitNode(sourceFile, visit) as ts.SourceFile;
    };

  // Transform the source file
  const result = ts.transform(sourceFile, [transformerFactory]);

  // Generate the new source file
  const printer = ts.createPrinter({ newLine: ts.NewLineKind.LineFeed });
  const transformedContent = printer.printFile(
    result.transformed[0] as ts.SourceFile
  );

  // Write the transformed content back to the file
  fs.writeFileSync(filePath, transformedContent, "utf-8");

  console.log(`Duplicate types removed from ${filePath}`);
}

// Usage
const filePath = path.join(process.cwd(), "gql", "graphql.ts");
removeDuplicateTypes(filePath);

[eddeee888]

This is going to be reverted.

Alpha version: @graphql-codegen/client-preset@4.5.0-alpha-20241025120637-8c227d2c67a14b063bb6a3f96d33069a98376541

Please let me know if this is working as expected and I can release it in the next version.

[LucidityDesign]

Works for me 👍

[eddeee888]

This is fixed in @graphql-codegen/client-preset@4.5.0. Thanks for your input everyone 🙏

Note that in the next major version, we will not be generating these internal types, nor allow onlyEnumTypes/onlyOperationTypes passthrough.

Please generate a separate generated type file target if you need to use it for other purposes:

  generates: {
    "src/graphql": {
      preset: 'client',
      // other config
    },
    "src/graphql/types.generated.ts": { // Use this file for non client preset purposes
      plugins: ['typescript']
    }
  }
}

[IgnisDa]

@eddeee888 Thanks for reverting the change. If possible, please include this information for in the release note for the next major release.

[leonardehrenfried]

Thanks for reverting the change.

I've now upgraded to 4.5.0 and added the new config snippet posted by @eddeee888: https://github.com/opentripplanner/OpenTripPlanner/pull/6221/files

I know that for 4.5.0 I don't actually need to change the config but will it just work then the next major version is released or is there anything else I need to do?

[eddeee888]

Hi @leonardehrenfried , in the future, it is recommended to generate other targets for non client preset purposes like this comment e.g.

// codegen.ts

// your other configs
  generates: {
    "src/graphql": {
      preset: 'client',
      // other config
    },
    "src/graphql/types.generated.ts": { // Use this file for non client preset purposes
      plugins: ['typescript']
    }
  }