Skip to content

Conversation

jack-guy
Copy link
Contributor

Following the release of TypeScript types in #355 there is one single error remaining in our internal service. It looks like TypeScript + ts-reset does not infer a string union type when using the getNewKeyTypeFromBatchKeySetType helper, according to this rule:

Make Set.has() less strict

src/graphql/dataloaders/__codegen__/foo_loaders.ts:x:y - error TS2345: Argument of type '{ foo: Set<string>; }' is not assignable to parameter of type 'Readonly<{ foo: Set<"BAR" | "BAZ">; }>'.
  Types of property 'foo' are incompatible.
    Type 'Set<string>' is not assignable to type 'Set<"BAR" | "BAZ">'.
      Type 'string' is not assignable to type '"BAR" | "BAZ"'.

1050                       ...__resourceArgs,
                           ~~~~~~~~~~~~~~~~~

Because of the reset override, TypeScript infers the parameter type of Set.prototype.has to be a string, rather than the generic type of the Set.

We can fix this by using a type inference instead, T extends Set<infer U> ? U : never;. This is arguably the more "correct" way to do it anyway.


After linking in the patched dataloader-codegen we no longer have any type errors in our dataloaders. I did not add a regression test as this is specific to users of ts-reset and the existing coverage from testExampleTypes/swapi.ts should cover the happy path.

@jack-guy jack-guy requested a review from magicmark July 11, 2025 20:57
@jack-guy jack-guy changed the title Fix Set key type for isBatchKeyASet Fix Set key type for isBatchKeyASet + ts-reset Jul 11, 2025
Copy link
Collaborator

@magicmark magicmark left a comment

Choose a reason for hiding this comment

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

makes sense to me, nice fix!

@jack-guy
Copy link
Contributor Author

This should have been released in 1.0.2 but I forgot to hit the merge button again 🙃

this will be 1.0.3 now. Sorry about that!

@jack-guy jack-guy merged commit 9932d41 into master Jul 11, 2025
1 check passed
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.

2 participants