[client-preset] allowed options and defaults

[charlypoly]

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

• nonOptionalTypename add nonOptionalTypename config #8702 - Request
• immutableTypes [client-preset] Forward immutableTypes option to client preset #10459 - Request

Current defaults

• presetConfig.fragmentMasking: true
• inlineFragmentTypes: true (if fragment masking is enabled)
• emitLegacyCommonJSImports: true
• inputMaybeValue: T | null | undefined

Requested defaults

• ignoreNoDocuments: true (@charlypoly)
• should presetConfig.fragmentMasking be false by default? (@charlypoly)
• dedupeFragments: true (duplicate fragments within queries #8103 (comment))
• enumsAsTypes: true (@n1ru4l)

[n1ru4l]

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

[n1ru4l]

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]

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?

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]

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]

Hi @n1ru4l. I requested support nonOptionalTypename. I use apollo client which adds a __typename to all its cached documents. In application code, I only use it in the context of file uploads. I have a REST api that handles file uploads and responds with a document-like response, something like

{ 
  photos: [ {id, filename}, {id, filename}, {id, filename} ] 
}

I then do an optimistic update of the object in apollo-client's cache, to which the files were uploaded, something like

const album = client.cache.identify({ __typename: 'Album', id: 1 });
album.photos = [ {id, filename}, {id, filename}, {id, filename} ]; // pseudo-code

To do the update above, I use client.cache.identify, which requires the object's __typename and its key field, in this case, id.

I know __typenames exist on my graphql documents and I want to indicate on the generated types that they do exist. Note that I'm not arguing for any default value for nonOptionalTypename, but rather the ability to configure it for myself.

EDIT: I don't have a lot of experience with different clients' behaviour wrt __typename. But if some clients don't add it, I think @n1ru4l's suggestion makes sense, that it should be omitted by default.

[joelmukuthu]

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]

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]

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

[ShravanSunder]

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]

@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]

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.