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

[client-preset] allowed options and defaults #8562

Open
charlypoly opened this issue Nov 2, 2022 · 42 comments
Open

[client-preset] allowed options and defaults #8562

charlypoly opened this issue Nov 2, 2022 · 42 comments
Assignees
Labels
feature-request help wanted Extra attention is needed

Comments

@charlypoly
Copy link
Contributor

charlypoly commented Nov 2, 2022

This issue is the place to discuss allowed options and default values when using the preset: 'client' setup.

Currently supported options

  • scalars
  • strictScalars
  • namingConvention
  • useTypeImports
  • skipTypename
  • enumsAsTypes
  • arrayInputCoercion
  • presetConfig.fragmentMasking
  • presetConfig.gqlTagName
  • presetConfig.unmaskFunctionName
  • emitLegacyCommonJSImports

Requested options support

Current defaults

  • presetConfig.fragmentMasking: true
  • inlineFragmentTypes: true (if fragment masking is enabled)
  • emitLegacyCommonJSImports: true

Requested defaults

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 2, 2022

useTypeImports should be on by default IMHO, as it is considered a best practice by me.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 2, 2022

nonOptionalTypename is dangerous IMHO - It allows the wrong configuration.

E.g. the correct type for

query MeQuery {
  me {
    id
  }
}

is

type MeQuery = {
  me: {
    id: string
  }
}

not

type MeQuery = {
  me: {
    __typename?: "User",
    id: string
  }
}

or

type MeQuery = {
  me: {
    __typename: "User",
    id: string
  }
}

However, GraphQL clients such as apollo and urql ADD __typename selections to the documents before sending them to the GraphQL server. I am unsure whether I like this behavior. But if you look at this it makes sense why the defaults are like they are today.

Having that said, for me, the following makes the most sense:

  • __typename is OMITTED from the generated types by default.

Adding the __typename property to the generated type via a configuration option by default, either as optional or mandatory - is something I would ideally try to avoid - but I can also see why people don't want to explicitly clutter their GraphQL Operation documents with __typename selections.

In order to make the apollo/urql users happy this means we will kind of need an automaticallyAddTypenameSelection configuration.

This is a potential proposal that groups the configuration under one object property:

type AutoTypenamesSelectionConfig = boolean | {
  skipRootTypes?: boolean
}

I don't event think we need to allow adding __typename properties as optional - it should either be non-optional or not at all present.

Curious to hear the use-cases of having __typename?: "User".

@marco2216
Copy link

Regarding fragmentMasking, in my understanding of why masking is implemented for Relay, it's in order to completely prevent other components from accessing data that they did not select (since it's not only on a type level, but actually the data is not accessible until the useFragment hook is called).

If data masking is only implemented on the type level, I'm not sure how useful it is. If we look at the reasoning behind the data masking that is stated in the Relay docs (see below), I think that as long as you have types generated at all, if we were to remove a property from one component that was used in another component, but wasn't specified in that component, it would be caught during type checking. If we are depending on it in a way that is not checkable by TypeScript, would it be caught if we had fragment masking on?
image

You could argue that it's more "correct" to force people to specify all the data deps for each component. And maybe there is a better chance that if you are forced to initially add all the data deps you need to the component itself, there is a lower chance that you will remove it by accident when changing another component. But it won't get enforced on the runtime level like Relay does it.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 2, 2022

If data masking is only implemented on the type level, I'm not sure how useful it is.

It helps a lot building scalable and isolated components and I use it even without runtime type masking. So to me it is very helpful.

Sometimes it is better to force people to use something that leads to better patterns.

I would prefer to have it on by default but allow people to opt-out if they really need to.

@joelmukuthu

This comment was marked as off-topic.

@joelmukuthu
Copy link

Sorry, my whole argument above doesn't apply to the generated types. The only reason I want nonOptionalTypename is to indicate that my documents do contain a non-optional __typename. But after giving this some more thought, I'll probably go with a skipTypename config instead. @charlypoly, feel free to remove nonOptionalTypename from the requested options (unless someone else requested it)

@sfarthin
Copy link

sfarthin commented Nov 7, 2022

I don't think enumsAsTypes is supported AFAICT. The only way I was able to use this option is to add this line: sfarthin@ae5cff7

Am I missing something?

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 8, 2022

@sfarthin We discussed internally that we want to set enumsAsTypes to true by default.

@ShravanSunder
Copy link

enumsAsConst: true,
maybeValue: 'T | undefined | null',
useTypenameImports: true,
avoidOptionals: true,

I would propose these. Its a common patter to use as const instead of enum in modern typescript.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 9, 2022

@ShravanSunder I agree with all your proposals - except maybeValue: 'T | undefined | null',. The maybe/value for nullable fields should be T | null - it can never be undefined.

@6uliver
Copy link

6uliver commented Nov 9, 2022

For maybe values this is how I prefer to use type settings:

avoidOptionals:
  field: true
  inputValue: false
  object: false
  defaultValue: true
maybeValue: T | null
inputMaybeValue: T | undefined

The reason for this configuration is that I can detect the difference in the returned data: if it is null it means that I requested it in the query or in a fragment so it is nulled by the server explicitly, if it is undefined (which can not be in theory because of the avoidOptionals and the typings unless I do some dynamic typing/indexing with any/unknown and casting) the field is missing from the response since I haven't requested it. If I would use both avoidOptionals with false for fields and maybeValue with null then I need to use types like this in components, functions, hooks and anything: T | null | undefined. The settings above are consistent, and I can expect T | null type in the returned data for GraphQL nullable types.

The other part is input data, I prefer to set inputMaybeValue to T | undefined and the avoidOptionals for inputValue and object to false since I don't want to set them to null explicitly in the code, I just leave them out so that they will be undefined. A great example is Relay's Cursor Connections Specification (https://relay.dev/graphql/connections.htm) where I don't want to set first/after and last/before together.

So instead of this

variables: {
    first: 10,
    after: 'abcd==',
    last: null,
    before: null,
}

I can write this

variables: {
    first: 10,
    after: 'abcd==',
}

Or for the first page I can write this (leaving out the after cursor too):

variables: {
    first: 10,
}

I think these are the defaults which make sense but I can imagine other coding styles or preferences.
Otherwise, based on these it would be great if these options can be configurable for the client-preset.
As I can see none of them is allowed and forwarded right now.

@charlypoly charlypoly pinned this issue Nov 16, 2022
@efreila
Copy link

efreila commented Nov 17, 2022

Currently using the client preset and am unable to get skipDocumentsValidation to work, although it works as expected with the same configuration for the fragment-matcher plugin:

module.exports = {
  schema: 'apps/server/src/graphql/schema/schema.graphql',
  documents: ['apps/client/src/**/!(*.d).{ts,tsx,js,jsx}'],
  ignoreNoDocuments: true, // for better experience with the watcher
  config: {
    skipDocumentsValidation: true,
  },
  generates: {
    'apps/client/src/graphql/': {
       preset: 'client',
       plugins: [],
    },
    'apps/client/src/graphql/fragment-matches.ts': {
      plugins: ['fragment-matcher'],
    },
  },
}

Are there any plans to support this option for client-preset?

@charlypoly
Copy link
Contributor Author

Currently using the client preset and am unable to get skipDocumentsValidation to work, although it works as expected with the same configuration for the fragment-matcher plugin:

module.exports = {
  schema: 'apps/server/src/graphql/schema/schema.graphql',
  documents: ['apps/client/src/**/!(*.d).{ts,tsx,js,jsx}'],
  ignoreNoDocuments: true, // for better experience with the watcher
  config: {
    skipDocumentsValidation: true,
  },
  generates: {
    'apps/client/src/graphql/': {
       preset: 'client',
       plugins: [],
    },
    'apps/client/src/graphql/fragment-matches.ts': {
      plugins: ['fragment-matcher'],
    },
  },
}

Are there any plans to support this option for client-preset?

Hi @efreila,

Could you provide a minimal repro?
The skipDocumentsValidation flag is handled at the core level of codegen, not at the plugin level, so it should work with all plugins and presets.

@aaronadamsCA
Copy link

I really think fragment masking should be disabled by default.

IMO the cost far outweighs any benefit: the extra types and hooks are a significant code smell, the hooks add unnecessarily runtime complexity, and interop is now much harder (how do I write Storybook stories now?). All this to prevent something structural types already handle perfectly well.

No thanks. If we wanted to use Relay, we would use Relay!

@saihaj
Copy link
Collaborator

saihaj commented Nov 28, 2022

I really think fragment masking should be disabled by default.
IMO the cost far outweighs any benefit: the extra types and hooks are a significant code smell, the hooks add unnecessarily runtime complexity, and interop is now much harder

Depends who you ask. I disagree there are lot of benefits fragment masking brings and working on large projects and trying to on-board someone I have found it useful.

how do I write Storybook stories now?

You can still mock your queries. Agreed there is some noise to make types happy.

@osdiab
Copy link

osdiab commented Dec 18, 2022

I would love some way to split apart the large generated typescript files into smaller chunks somehow (eg by fragments and queries found in code; or by some simple way to mash the near-operation-preset with the client-preset, which idk how I would do it), so that the typescript compiler with the incremental flag on can better cache code that won’t change when the schema changes. I use Hasura, the schema it outputs can get very large as the project grows, and since I mostly get a giant output file, any change forces typescript to reevaluate all of the schema types, which causes updates to lock up for a while and VSCode performance to tank if I make the mistake of looking at the generated gql file.

@ojab
Copy link

ojab commented Dec 23, 2022

What about futureProofEnums & futureProofUnions? I guess it's not possible to make them defaults because it will break many codebases, but I prefer to have them enabled.

@saihaj
Copy link
Collaborator

saihaj commented Dec 26, 2022

I would love some way to split apart the large generated typescript files into smaller chunks somehow (eg by fragments and queries found in code; or by some simple way to mash the near-operation-preset with the client-preset, which idk how I would do it), so that the typescript compiler with the incremental flag on can better cache code that won’t change when the schema changes. I use Hasura, the schema it outputs can get very large as the project grows, and since I mostly get a giant output file, any change forces typescript to reevaluate all of the schema types, which causes updates to lock up for a while and VSCode performance to tank if I make the mistake of looking at the generated gql file.

@osdiab you can try adding these two options and that should help shrink size of autogenerated file.

preResolveTypes: true,
onlyOperationTypes: true

There is a bigger performance improvement change which I have a discussion for here #8693 which will help even more.

@maclockard
Copy link
Contributor

dedupeFragments set to true currently breaks write/readFragment so I'm not sure it can be turned on by default. see: #8582

@tonypee
Copy link

tonypee commented Mar 9, 2023

I am wanting to set 'maybeValue' - can this be added too? It seems silly to not include all of the properties of the underlying plugins, otherwise you cant setup your project as previously!
If the reasoning is that 'maybeValue' isnt needed - then why was it there before? this is disappointing, as i need this to work

@n1ru4l
Copy link
Collaborator

n1ru4l commented Mar 9, 2023

Why do you want to set maybeValue?

@tonypee
Copy link

tonypee commented Mar 10, 2023

Dont worry, i can do it what i need with:
avoidOptionals: {
field: true,
inputValue: false,
object: false,
defaultValue: false,
},
To have the fields allow optional, while the values are T | null. This seems like the best setup for me. I still dont understand why the default makes the values T | null | undefined.

Thanks for the great lib & new feature (if it is new!)

@pingiun
Copy link

pingiun commented Apr 13, 2023

having queries return values that are string | null | undefined is very annoying. this could previously be solved with maybeValue: T. now i have to do value?: string | null everywhere or coalesce null values everywhere

@prscoelho
Copy link

Does unmaskFunctionName default name really have to be a hook? From the looks of it, it's not actually a hook, but if the function name starts with use then tooling will complain if it's used conditionally.

Setting the config below fixes the problem, but maybe this could be the default?

presetConfig: {
    fragmentMasking: { unmaskFunctionName: 'getFragmentData' },
},

@MH4GF
Copy link

MH4GF commented Apr 24, 2023

I want to use enumsAsConst with client-preset.
I want to treat them as object values, not types, e.g. typescript-mock-data,typescript-validation-schema refers to a value.


I know that client-preset is intentionally limiting the options, so I am not directly publishing PRs, but sharing use cases.
I am currently resolving this by outputting the typescript plugin results as a separate file, but I feel that this creates duplication in a subtle way.

@omavi
Copy link

omavi commented Apr 28, 2023

having queries return values that are string | null | undefined is very annoying. this could previously be solved with maybeValue: T. now i have to do value?: string | null everywhere or coalesce null values everywhere

Very much agreed. What do you mean by previously? Is there an old way to configure that I can revert back to, in order to leverage maybeValue: T, but keep the rest of the config we get with client-preset?

@sampsonjoliver
Copy link

+1 to enumAsConst.

@ArnaudD
Copy link

ArnaudD commented Jul 20, 2023

+1 for enumValues. I'm using TS string enums in my codebase and they don't match the enums generated by the client-preset without this option.

enum EnumFromCodegen {
  Dog = "dog",
  Cat = "cat"
}

enum EnumFromCodebase {
  Dog = "dog",
  Cat = "cat"
}

function foo(pet: EnumFromCodebase) {
  return pet;
}

foo(EnumFromCodegen.Dog);
//  ^ Argument of type 'EnumFromCodegen.Dog' is not assignable to parameter of type 'EnumFromCodebase'.

@Stephen2
Copy link

Stephen2 commented Jul 28, 2023

I'm not understanding why enumsAsConst isn't allowed in this preset.

EDIT: I think I'm going to (gross) use patch-package to allow enumsAsConst. It just works, and fixes all my enum related challenges

@schmod
Copy link
Contributor

schmod commented Aug 29, 2023

The skipDocumentsValidation flag is handled at the core level of codegen, not at the plugin level, so it should work with all plugins and presets.

I don't think this is true, and I was also unable to use that setting.

I've opened #9653, which adds a line of code that allows that option to be passed upstream to where it needs to go.

@charpeni
Copy link
Contributor

I created a pull request to forward futureProofEnums because I believe it is super useful to treat enums as open-ended: #9683.

I also wrote about it here: 📚 GraphQL Enums Are Unsafe.

@WonderPanda
Copy link

I'm just getting caught up with all the latest and greatest in the ecosystem. Overall I'm really excited about this direction but one thing that I feel like I'm missing right now is the ability to leverage the import-types preset with the new client-preset setup.

I also use Hasura so I have a very big schema and I like to have the base types for the schema code-generated into their own library in my NX monorepos. That way I can keep operations in another lib and just import my core schema types from eg @shared/graphql-types.

Additionally I sometimes have multiple apps or feature libraries inside a monorepo. With my normal approach I only need to have one copy of the types for the Schema and then each app or lib owns its own operation types but with the new client preset it seems like I'm going to get multiple copies of all the types from the whole schema for each one

I think this would also help out with the situation @osdiab is describing.

It looks like the preset is just a collection of plugins composed so I can probably reverse engineer and build my own that allows me to use import-types but it would be great if this were supported out of the box instead

@Stephen2
Copy link

Stephen2 commented Dec 6, 2023

Seem like link is another config option no longer passed through this preset, to other plugins. This is preventing me from passing link to the typescript-msw plugin, and rendering it useless for me :(

I've again used patch-package to pass this through, and it worked absolutely fine so far.

@ii64
Copy link

ii64 commented Dec 13, 2023

What is the status of this change? skipDocumentsValidation still ignored when using client-preset, comments mentioned that it is implemented in the core level of codegen, we do have https://github.com/dotansimha/graphql-code-generator/pull/9653/files - it caused by the option is not being forwareded so I'd like to know why is the PR is not getting a merge?

✔ Parse Configuration
⚠ Generate outputs
  ❯ Generate to src/generated/gql/main/
    ✔ Load GraphQL schemas
    ✔ Load GraphQL documents
    ✖ GraphQL Document Validation failed with 5 errors;

@seadub
Copy link

seadub commented Dec 19, 2023

+1 to above, experiencing same issue right now while integrating with 3rd party API which apparently doesn't follow UniqueInputFieldNamesRule. Attempts to skip validation with config at various levels is failing.

@2snEM6
Copy link

2snEM6 commented Dec 24, 2023

+1 to above as well, skipDocumentsValidation and flattenGeneratedTypes are needed in order for the Relay Operation Optimizer to work. Are those two options compatible with client-preset?

@davlet61
Copy link

davlet61 commented Jan 11, 2024

Would be nice to have maybeValue and disableDescriptions options as well, don't seem work

@azu
Copy link

azu commented May 29, 2024

enumsAsConst

The current default output is TypeScript's enum.
However, TypeScript's enum is a proprietary feature of TypeScript, so I prefer to avoid using it.

enumAsType provides equivalent functionality at the type level, but enumsAsConst is preferred for ease of code review.

Output

GraphQL:

enum Color {
  RED
  BLUE
  GREEN
}

Default output:

export enum Color {
  RED = 'RED',
  BLUE = 'BLUE',
  GREEN = 'GREEN',
}

enumAsType output:

export type Color = 'RED' | 'BLUE' | 'GREEN';

enumsAsConst output:

export const Color = {
  RED: 'RED',
  BLUE: 'BLUE',
  GREEN: 'GREEN',
} as const;
export type Color = typeof Color[keyof typeof Color];

Usage

Default output:

import { Color } from './generated/graphql';

const color = "...";
if (color === Color.RED) {
  console.log('Red color');
}

enumAsType output:

const color = "...";
if (color === 'RED') {
  console.log('Red color');
}

enumsAsConst output:

import { Color } from './generated/graphql';

const color = "...";
if (color === Color.RED) {
  console.log('Red color');
}

enumAsType enforces the use of string literals, which miss a context of Color type.
When I review the code, I need to think about "RED" string literal is a valid value of Color or not.
(type check is enough, but it is a little bit hard to know the context of Color type when I review the code.)

Default output and enumsAsConst output provide the context of Color type.
So, I want to use enumsAsConst option.

@ParadiseFallen
Copy link

can we add generateUseFragments so it would generate it for us?

fragment UserCard_User on User
{
  name
  age
}
function useUserCard_UserFragment(/*dunno about type of keys*/)
{
  return useFragment(/*...*/)
}

@brian-cirieco
Copy link

I would like the ability to rename the masking function makeFragmentData.

From the recommended nickname of the masking function getFragmentData, the meaning of unmasking a fragment is to expose/get the data, whereas makeFragmentData sounds very much the same, when it is actually making a fragment from data.

In other words:

  • unmasking the fragment === get the data from the fragment === getFragmentData
  • masking fragment data === mask the data === maskFragmentData or makeFragment

@mkosir
Copy link

mkosir commented Aug 22, 2024

Awesome list of options and defaults proposal!
Just wondering if instead of waiting to include immutableTypes in v5, could we already do it in current version v4, since it wouldn't be considered as breaking change, or would it 🤔?
I'm already using immutableTypes and it's great option to have, just last piece is missing for client-preset. Keep up the great work!

@aszenz
Copy link

aszenz commented Sep 26, 2024

Also add isolatedDeclarations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests