[typescript-resolvers] Extract union types to ResolversUnionTypes #9069

eddeee888 · 2023-02-26T05:34:50Z

Description

As discussed in a proof of concept, we thought that by creating a ResolversUnionTypes type and use reference unions in ResolversTypes and ResolversParentTypes give us a bit more control in the future.

As the result, the union types are no longer inlined in ResolversTypes and ResolversParentTypes

Example use case (future)

In the future, we could make union members' __typename non-nullable whilst leaving the counterparts in ResolversTypes and ResolversParentTypes as optional:

// typescript plugin types
export type BookResult = {
  __typename?: "BookResult";
  result?: Maybe<Book>;
};

export type StandardError = {
  __typename?: "StandardError";
  error: string;
};

// typescript-resolvers types
export type ResolversUnionTypes = {
  BookPayload: (BookResult & { __typename: "BookResult" }) | (StandardError & { __typename: "StandardError" }); // Non-nullable typename are not implemented. This is here as proof of concept
};

export type ResolversTypes = {
  // ...
  BookPayload: ResolverTypeWrapper<ResolversUnionTypes["BookPayload"]>; // This forces `BookPayload` require __typename from its union members `BookResult` and `StandardError`
  BookResult: ResolverTypeWrapper<BookResult>; // __typename of BookResult is still optional
  StandardError: ResolverTypeWrapper<StandardError>;
  // ...
};

export type ResolversParentTypes = {
  // ...
  BookPayload: ResolversUnionTypes["BookPayload"]; // This means BookPayload.__resolveType has access to non-nullable __typename from its union members.
  BookResult: BookResult;
  StandardError: StandardError;
  // ...
};

Type of change

Please delete options that are not relevant.

Refactor: change how union types are referenced

How Has This Been Tested?

Unit tests

Test Environment:

OS: MacOS
@graphql-codegen/typescript-resolvers:
NodeJS: 16.15.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

changeset-bot · 2023-02-26T05:34:53Z

🦋 Changeset detected

Latest commit: 7758126

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

This PR includes changesets to release 9 packages

Name	Type
@graphql-codegen/visitor-plugin-common	Patch
@graphql-codegen/typescript-resolvers	Patch
@graphql-codegen/typescript-document-nodes	Patch
@graphql-codegen/gql-tag-operations	Patch
@graphql-codegen/typescript-operations	Patch
@graphql-codegen/typed-document-node	Patch
@graphql-codegen/typescript	Patch
@graphql-codegen/graphql-modules-preset	Patch
@graphql-codegen/client-preset	Patch

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

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

eddeee888 · 2023-02-26T05:41:42Z

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

@@ -880,6 +876,82 @@ export class BaseResolversVisitor<
    return `Array<${t}>`;
  }

+  protected createResolversUnionTypes(): Record<string, string> {


createResolversUnionTypes is the simplified version of createResolversFields that is targeted for Unions.
I chose to create a new function because createResolversFields is quite large and rigid so it's hard to implement/maintain

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

eddeee888 · 2023-02-26T05:50:19Z

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

+
+          return unionMemberValue;
+        });
+        res[typeName] = referencedTypes.map(type => `( ${type} )`).join(' | '); // Must wrap every union member in explicit "( )" to separate the members


Must wrap every union member in explicit "( )" to separate the members

Result usually looks like this:

export type ResolversUnionTypes = { UnionThing: ( TypeA ) | ( TypeB ) }

Prettier should remove the round brackets automatically i.e. result will be UnionThing: TypeA | TypeB

I've added it this way so it's a bit easier to add types to the union members without ambiguity e.g.

// This is ambiguous UnionThing: TypeA & { __typename: 'TypeA' } | TypeB & { __typename: 'TypeB' } // Prettier will try to turn it into this: UnionThing: ( TypeA & { __typename: 'TypeA' } ) | ( TypeB & { __typename: 'TypeB' } )

eddeee888 · 2023-02-26T05:52:40Z

packages/plugins/typescript/resolvers/tests/ts-resolvers.spec.ts

@@ -1426,10 +1426,38 @@ export type ResolverFn<TResult, TParent, TContext, TArgs> = (
      union CCCUnion = CCCFoo | CCCBar
    `);

-    const tsContent = (await tsPlugin(testSchema, [], {}, { outputFile: 'graphql.ts' })) as Types.ComplexPluginOutput;
+    const tsContent = await tsPlugin(testSchema, [], {}, { outputFile: 'graphql.ts' });


tsContent should be typed as Types.ComplexPluginOutput so we don't need to do as Types.ComplexPluginOutput.

Happy to put this back if it was here for some particular reasons

eddeee888 · 2023-02-26T05:55:38Z

packages/plugins/typescript/resolvers/tests/mapping.spec.ts

+    expect(result.content).toBeSimilarStringTo(`
+      export type ResolversParentTypes = {
+        MyType: Omit<MyType, 'unionChild'> & { unionChild?: Maybe<ResolversParentTypes['ChildUnion']> };
+        String: Scalars['String'];
+        Child: Child;
+        MyOtherType: MyOtherType;
+        ChildUnion: ResolversUnionTypes['ChildUnion'];
+        Query: {};
+        Subscription: {};
+        Node: ResolversParentTypes['SomeNode'];
+        ID: Scalars['ID'];
+        SomeNode: SomeNode;
+        MyUnion: ResolversUnionTypes['MyUnion'];
+        MyScalar: Scalars['MyScalar'];
+        Int: Scalars['Int'];
+        Boolean: Scalars['Boolean'];
+      };
+    `);


Added ResolversParentTypes to all related tests in this file because I want to be sure that ResolversUnionTypes is being used correctly to reference.

eddeee888 · 2023-02-26T06:01:16Z

packages/plugins/typescript/resolvers/tests/mapping.spec.ts

    expect(result.content).toBeSimilarStringTo(`
    export type ResolversTypes = {
      MyType: ResolverTypeWrapper<Omit<MyType, 'unionChild'> & { unionChild?: Maybe<ResolversTypes['ChildUnion']> }>;
      String: ResolverTypeWrapper<Scalars['String']>;
      Child: ResolverTypeWrapper<Child>;
      MyOtherType: ResolverTypeWrapper<MyOtherType>;
-      ChildUnion: ResolversTypes['Child'] | ResolversTypes['MyOtherType'];
+      ChildUnion: ResolverTypeWrapper<ResolversUnionTypes['ChildUnion']>;


Previously, this is ResolversTypes['Child'] | ResolversTypes['MyOtherType'] which translate to:

ResolverTypeWrapper<Child> | ResolverTypeWrapper<MyOtherType>

Now, using ResolverTypeWrapper<ResolversUnionTypes['ChildUnion']> translate to:

ResolverTypeWrapper<Child | MyOtherType>

I think these 2 are the same semantically. Happy to hear explanation if they are not 🙂

(This is applicable to all updated tests in this file)

eddeee888 · 2023-02-26T06:09:39Z

packages/plugins/typescript/resolvers/tests/mapping.spec.ts

+    expect(result.content).toBeSimilarStringTo(`
+      export type ResolversUnionTypes = {
+        ChildUnion: ( Omit<Child, 'parent'> & { parent?: Maybe<ResolversTypes['MyType']> } ) | ( MyOtherType );
+        MyUnion: ( CustomPartial<Omit<MyType, 'unionChild'> & { unionChild?: Maybe<ResolversTypes['ChildUnion']> }> ) | ( MyOtherType );


This could be related to #9043 as well.

Ideally, the MyType is should be treated the same way as ResolversParentTypes['MyType']. However:

The ResolversParentTypes['MyType'] is using the type from typescript plugin.

The mentioned issue seems to suggest that we should use the "Omit type" instead

Currently, ResolversUnionTypes is using "Omit type" to replace the { T } placeholder here but happy to change to match whichever direction ResolversParentTypes['MyType'] uses.

cc @saihaj @gilgardosh

FYI, according to #1631, the parent type should use the "Omit type", and #1677 implements it. Not sure when it broke between then and now, but I suspect it was unintentional.

@eddeee888 awesome work!
I implemented a fix here #9110 , but it's conflicting. I'll adjust rebase it on top of yours after it's merged

eddeee888 · 2023-02-28T08:57:03Z

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts

+        const originalTypeName = isScalar ? this._getScalar(typeName) : prev[typeName];

-        // Don't wrap Union with ResolverTypeWrapper, each inner type already has it
        if (isUnionType(schemaType)) {
-          prev[typeName] = replaced;
+          // Don't clear ResolverTypeWrapper from Unions
+          prev[typeName] = replacePlaceholder(this.config.defaultMapper.type, originalTypeName);
        } else {
-          prev[typeName] = applyWrapper(replacePlaceholder(this.config.defaultMapper.type, name));
+          const name = clearWrapper(originalTypeName);
+          const replaced = replacePlaceholder(this.config.defaultMapper.type, name);
+          prev[typeName] = applyWrapper(replaced);


Previous logic suggests that it's possible for a union to be in the scalar map 🤔 I'm unsure when/if it's possible.

However, the updated logic should be backward compatible as it generates the same partial and wrapper order.

…ionTypes

…pes using this.convertName

Since unions are now treated separately, instead of inline referenced, we do not need to avoid wrapping ResolverTypeWrapper. However, the result now is ResolverTypeWrapper<Partial< A | B >> instead of: Partial<ResolverTypeWrapper<A> | ResolverTypeWrapper<B> > But... maybe it's ok? 🤔

- Formatting - [ ] Abstract duplicated code

…port correctly

hector-del-rio · 2023-03-14T11:53:27Z

This caused an issue in my project due to the fact that now the first argument for __resolveType resolver function definition is wrapped in ResolverTypeWrapper where before it wasn't, which makes it possibly a promise.

ryanrhee · 2023-03-14T15:57:34Z

type ResolverTypeWithoutPromise = Exclude<ResolverTypeWrapper, Promise<unknown>> should do what you want.

ryanrhee · 2023-03-14T15:59:58Z

you can also extract all the sub-types as well.

import type { ResolversTypes } from "./path/to/generated/graphql"

type ShallowTypes = {
  [Key in keyof ResolversTypes]: Exclude<ResolversTypes[Key], Promise<unknown>>
}

hector-del-rio · 2023-03-15T07:17:24Z

@ryanrhee thanks for your answer. The issue is with the parent argument (first argument) of the resolver for __resolveType field.

export type ResolversParentTypes = ResolversObject<{
  ...
  MyType: ResolversUnionTypes['MyType'];
  AnotherType: ResolverTypeWrapper<Mapper>
  ...
}>;

export type ResolversUnionTypes = ResolversObject<{
  MyType: { contract: ResolversTypes['AnotherType'] }; // This now uses ResolversTypes which uses ResolverTypeWrapper
}>;

I guess the only way to fix it is to add this type to the mappers as well?

ryanrhee · 2023-03-15T16:41:48Z

You could add it to the mappers, yes.

Does __resolveType() allow async functions? If so, you could check if the first argument is a promise and then resolve it so that you are always operating on the underlying type.

{
  __resolveType: async (objOrPromise, ...) => {
    // objOrPromise is `Promise<T> | T`
    const obj = Promise.resolve(objOrPromise)
    // your code here; obj is now of type `T`
  }
}

hector-del-rio · 2023-03-16T06:50:49Z

Thanks @ryanrhee. I needed to add await, but it worked:

{
  __resolveType: async (objOrPromise, ...) => {
    // objOrPromise is `Promise<T> | T`
    const obj = await Promise.resolve(objOrPromise)
    // your code here; obj is now of type `T`
  }
}

However, it is a bit concerning that this happened in a patch update from @graphql-codegen/typescript-resolvers 3.1.0 to 3.1.1.

eddeee888 · 2023-03-16T12:18:10Z

Hi @hector-del-rio ,

Do you mind creating a small repo or sandbox with a small schema and minimal codegen config? This will help me understand the problem better 🙂

For context, this went in as a patch because we only move type references around. Maybe I missed a use case. Sorry!

eddeee888 · 2023-03-21T10:38:49Z

Hi @hector-del-rio,

This should be fixed in #9206 🙂
It'll go out in the next version. Thanks for reporting the issue 🙌

hector-del-rio · 2023-03-22T16:55:20Z

Thank you @eddeee888 🙏

eddeee888 commented Feb 26, 2023

View reviewed changes

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts Outdated Show resolved Hide resolved

eddeee888 commented Feb 26, 2023

View reviewed changes

packages/plugins/other/visitor-plugin-common/src/base-resolvers-visitor.ts Outdated Show resolved Hide resolved

eddeee888 commented Feb 26, 2023

View reviewed changes

eddeee888 force-pushed the typescript-resolvers-refactor-union-types branch from a65e065 to 4291915 Compare February 26, 2023 06:11

eddeee888 commented Feb 28, 2023

View reviewed changes

eddeee888 added 11 commits February 28, 2023 20:00

Build ResolversUnionTypes without referencing it

dc52ffe

Update ResolversTypes and ResolversParentTypes to link to ResolversUn…

cc5b934

…ionTypes

Ensure schema type names are referenced correctly in ResolversUnionTy…

77592f5

…pes using this.convertName

Do not generate ResolversUnionTypes if there is no union

02d2624

Do not test comment generated for ResolversUnionTypes

92445a0

Fix Omit in ResolversUnionTypes and tests

669ef3b

Clean up

2caf69b

- Formatting - [ ] Abstract duplicated code

Abstract getRelevantFieldsToOmit

43cffcf

Ensure backward compatibility when using defaultMappers with placeholder

869aef4

Ensure union types go through federation and handle applyNamespacedIm…

09b4d59

…port correctly

eddeee888 force-pushed the typescript-resolvers-refactor-union-types branch from 89a4288 to 09b4d59 Compare February 28, 2023 09:01

eddeee888 marked this pull request as ready for review February 28, 2023 09:01

Fix dev-test

6dd816c

n1ru4l requested review from n1ru4l and gilgardosh February 28, 2023 09:15

n1ru4l approved these changes Mar 1, 2023

View reviewed changes

gilgardosh approved these changes Mar 5, 2023

View reviewed changes

Add changeset

a2d0369

eddeee888 added 2 commits March 6, 2023 21:36

Merge branch 'master' into typescript-resolvers-refactor-union-types

21132ad

Remove comment

7758126

eddeee888 merged commit 4b49f6f into dotansimha:master Mar 6, 2023

eddeee888 deleted the typescript-resolvers-refactor-union-types branch March 6, 2023 11:27

theguild-bot mentioned this pull request Mar 6, 2023

Upcoming Release Changes #9054

Merged

eddeee888 mentioned this pull request Mar 10, 2023

[typescript-resolvers] Add resolversNonOptionalTypename config #9146

Merged

12 tasks

theguild-bot mentioned this pull request Mar 19, 2023

Upcoming Release Changes #9132

Merged

eddeee888 mentioned this pull request Mar 20, 2023

[typescript-resolvers] Fix resolvers union types used in ResolversParentTypes #9206

Merged

12 tasks

eddeee888 mentioned this pull request Mar 26, 2023

[testing] Add complex Interface test cases for typescript-resolvers #9228

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript-resolvers] Extract union types to ResolversUnionTypes #9069

[typescript-resolvers] Extract union types to ResolversUnionTypes #9069

eddeee888 commented Feb 26, 2023 •

edited

Loading

changeset-bot bot commented Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023

ryanrhee Mar 3, 2023

gilgardosh Mar 5, 2023

eddeee888 Feb 28, 2023

hector-del-rio commented Mar 14, 2023

ryanrhee commented Mar 14, 2023

ryanrhee commented Mar 14, 2023

hector-del-rio commented Mar 15, 2023

ryanrhee commented Mar 15, 2023 •

edited

Loading

hector-del-rio commented Mar 16, 2023

eddeee888 commented Mar 16, 2023

eddeee888 commented Mar 21, 2023

hector-del-rio commented Mar 22, 2023

[typescript-resolvers] Extract union types to ResolversUnionTypes #9069

[typescript-resolvers] Extract union types to ResolversUnionTypes #9069

Conversation

eddeee888 commented Feb 26, 2023 • edited Loading

Description

Example use case (future)

Type of change

How Has This Been Tested?

Checklist:

changeset-bot bot commented Feb 26, 2023 • edited Loading

🦋 Changeset detected

eddeee888 Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

eddeee888 Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

eddeee888 Feb 26, 2023

Choose a reason for hiding this comment

eddeee888 Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

eddeee888 Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

eddeee888 Feb 26, 2023

Choose a reason for hiding this comment

ryanrhee Mar 3, 2023

Choose a reason for hiding this comment

gilgardosh Mar 5, 2023

Choose a reason for hiding this comment

eddeee888 Feb 28, 2023

Choose a reason for hiding this comment

hector-del-rio commented Mar 14, 2023

ryanrhee commented Mar 14, 2023

ryanrhee commented Mar 14, 2023

hector-del-rio commented Mar 15, 2023

ryanrhee commented Mar 15, 2023 • edited Loading

hector-del-rio commented Mar 16, 2023

eddeee888 commented Mar 16, 2023

eddeee888 commented Mar 21, 2023

hector-del-rio commented Mar 22, 2023

eddeee888 commented Feb 26, 2023 •

edited

Loading

changeset-bot bot commented Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

eddeee888 Feb 26, 2023 •

edited

Loading

ryanrhee commented Mar 15, 2023 •

edited

Loading