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

resolve makeFragmentData type problem by creating unmask Fragment utility type #9708

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tnyo43
Copy link
Contributor

@tnyo43 tnyo43 commented Oct 15, 2023

Description

Related #9702
also related to #9380

makeFragmentData can't make a mock data with a fragment includes multiple fragments (see #9702 ).
I created UnmaskFragment and apply it to the first argument of the function to solve the problem.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

This might be a breaking change if you are using nested makeFragmentData to create a mock data for a nested fragment as like follows:

makeFragmentData({
  bar: makeFragmentData({ ... }, BarFragment)
}, SomeFragment);

Screenshots/Sandbox (if appropriate/relevant):

sandbox of testing the UnmaskingFragment utility type

How Has This Been Tested?

  1. check the failing code with the current makeFragmentData at a sandbox
    1.1. clone https://github.com/tnyo43/graphql-code-generator-issue-fragment-conflict and move to the root of it.
    1.2. run pnpm run install & pnpm run generate.
    1.3. open "./src/User.tsx" and check the mockData is not able to be typed without any type assertion.
    • this is because User_UserFragment includes multiple fragment.
  2. prepare this repository
    2.1 clone this repository and checkout to this branch (tnyo43:update-make-fragment-data-args-type)
    2.2 run yarn install & yarn build
  3. apply the change to the sandbox (see Apply fixed make fragment data tnyo43/graphql-code-generator-issue-fragment-conflict#1)
    3.1. move to sandbox and update the dependency of "@graphql-codegen/client-preset" to refer to the local change (ex. "@graphql-codegen/client-preset": "file:../graphql-code-generator/packages/presets/client").
    3.2. run pnpm run install & pnpm run generate.
    3.3. open "./src/User.tsx" and update mockData as like follows. You will see that we don't need any extra type assertion for making mock data!
const mockData = makeFragmentData(
  {
    id: "user_1",
    username: "aaa",
    avatarUrl: "aaa",
    email: "mail@mail.com",
  },
  User_UserFragment
);

Test Environment:

  • OS: macOS 14.0 (23A344)
  • @graphql-codegen/cli: 4.0.1
  • @graphql-typed-document-node/core: 3.2.0,
  • typescript: 5.2.2
  • NodeJS: 18.16.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
  • [N/A] 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

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@changeset-bot
Copy link

changeset-bot bot commented Oct 15, 2023

🦋 Changeset detected

Latest commit: 015cefe

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

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/client-preset Minor

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

@tnyo43 tnyo43 marked this pull request as draft October 15, 2023 08:58
packages/presets/client/src/fragment-masking-plugin.ts Outdated Show resolved Hide resolved
Comment on lines -128 to +172
const documentNodeImport = `${useTypeImports ? 'import type' : 'import'} { ResultOf, DocumentTypeDecoration${
const documentNodeImport = `${useTypeImports ? 'import type' : 'import'} { DocumentTypeDecoration${
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, we don't use ResultOf utility type in this file any more.

} & (F extends { " $fragmentRefs"?: { [K in string]: infer FRefs }; }
? (FRefs extends any ? Flatten<FRefs> : never) : {}
);
export type UnmaskFragment<F> = UnionFieldToIntersection<Flatten<F>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UnmaskFragment utility type consists of two utility types, Flatten and UnionFieldToIntersection.

Flatten refers to ' $fragmentRefs' properties and returns a new type by making the all sub-fragment flat recursively.
it returns a union type of fragments if the fragment includes multiple fragments in the same field. So we use UnionFieldToIntersection to transform it to an intersection type of them.

I made a playground, so you will be able to see how they actually works.

Comment on lines +27 to +33
type UnionToIntersectGroupByTypeName<U, V = U> = [V] extends [
{ __typename?: infer TypeName }
]
? TypeName extends any
? UnionToIntersection<U extends { __typename?: TypeName } ? U : never>
: never
: never;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to make the union of fragment type into an intersection type of them if and only if they have same __typename value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made something like type tests of makeFragmentData.
Unfortunately, it seems there is no type checking in CI process. So there is no rational way to check the type of makeFragmentData is correct.
If you know of a better way, please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a small example of makeFragmentData with nested fragment.
Actually it is not able to be typed with the previous implementation of makeFragmentData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some field for tests of makeFragmentData.

Copy link
Contributor Author

@tnyo43 tnyo43 left a comment

Choose a reason for hiding this comment

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

A lot of changes are made by running scripts.
I left comments to at the files I changes. You can just ignore other files for quick review.

Comment on lines -19 to +64
>(data: FT, _fragment: F): FragmentType<F> {
return data as FragmentType<F>;
>(data: UnmaskFragment<FragmentType<F>>, _fragment: F): FragmentType<F> {
return data as any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main update of this PR is here.
ResultOf<F> does not accept any object if the fragment includes multiple fragments (I made an example #9702 ).
I made UnmaskFragment utility type to make it able to accept valid data for the fragment.

@tnyo43 tnyo43 force-pushed the update-make-fragment-data-args-type branch from a7b8032 to fcd0129 Compare October 15, 2023 14:37
tnyo43 added a commit to tnyo43/graphql-code-generator-issue-fragment-conflict that referenced this pull request Oct 15, 2023
you need to prepare clone and build the repository
@tnyo43 tnyo43 changed the title Update make fragment data args type resolve makeFragmentData type problem with creating unmask Fragment utility type Oct 15, 2023
@tnyo43 tnyo43 changed the title resolve makeFragmentData type problem with creating unmask Fragment utility type resolve makeFragmentData type problem by creating unmask Fragment utility type Oct 15, 2023
tnyo43 added a commit to tnyo43/graphql-code-generator-issue-fragment-conflict that referenced this pull request Oct 15, 2023
you need to prepare clone and build the repository
@tnyo43
Copy link
Contributor Author

tnyo43 commented Oct 16, 2023

@tnyo43 tnyo43 marked this pull request as ready for review October 16, 2023 13:37
@saihaj saihaj requested review from beerose and n1ru4l October 19, 2023 14:45
@tnyo43
Copy link
Contributor Author

tnyo43 commented Feb 23, 2024

@saihaj Could you please provide some advice on how to approach this?
As I mentioned in the description, I know it is kind a breaking change so we may need to discuss a lot to merge.

tnyo43 added 10 commits March 8, 2024 18:49
it is not required since the previous commit
run `yarn build & yarn generate:examples`
gql function seems not to be used since dotansimha#9217 is completed
this mock data is what is not able to be typed with the previous `makeFragmentData` function.
That is because `TweetsFragment` includes two different fragments
run `yarn examples:codegen`
@tnyo43 tnyo43 force-pushed the update-make-fragment-data-args-type branch from fcd0129 to 015cefe Compare March 8, 2024 09:58
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.

None yet

1 participant