FCT 598: Omit __typename field when building graphql draft models

[ByronDWall]

When transforming fields for graphql draft models, built fields will add a __typename field, which is not a valid field to send in a draft and makes API's very sad and send us a 400.

This PR implements an isGraphqlDraft option that can be passed to the Transformer function. When this option is passed, all __typename fields are removed from the transformed model so that the model can be used directly as a draft.

For a more in depth walkthrough and explanation of this code, see this previous PR.

[changeset-bot]

🦋 Changeset detected

Latest commit: 5a234a9

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

This PR includes changesets to release 33 packages

Name	Type
@commercetools-test-data/core	Minor
@commercetools-test-data/associate-role	Minor
@commercetools-test-data/business-unit	Minor
@commercetools-test-data/cart-discount	Minor
@commercetools-test-data/cart	Minor
@commercetools-test-data/category	Minor
@commercetools-test-data/channel	Minor
@commercetools-test-data/commons	Minor
@commercetools-test-data/custom-object	Minor
@commercetools-test-data/custom-view	Minor
@commercetools-test-data/customer-group	Minor
@commercetools-test-data/customer	Minor
@commercetools-test-data/discount-code	Minor
@commercetools-test-data/inventory-entry	Minor
@commercetools-test-data/order	Minor
@commercetools-test-data/payment	Minor
@commercetools-test-data/product-discount	Minor
@commercetools-test-data/product-selection	Minor
@commercetools-test-data/product-type	Minor
@commercetools-test-data/product	Minor
@commercetools-test-data/quote-request	Minor
@commercetools-test-data/quote	Minor
@commercetools-test-data/review	Minor
@commercetools-test-data/shipping-method	Minor
@commercetools-test-data/shopping-list	Minor
@commercetools-test-data/staged-quote	Minor
@commercetools-test-data/state	Minor
@commercetools-test-data/store	Minor
@commercetools-test-data/tax-category	Minor
@commercetools-test-data/type	Minor
@commercetools-test-data/zone	Minor
@commercetools-test-data/graphql-types	Minor
@commercetools-test-data/utils	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

[jaikamat]

Is there any use-case for providing the draft __typename from a consumer perspective? It's obviously a field that's needed to conform to a GraphQL schema since __typename is necessary, but could this functionality be made automatic?

I'm thinking about the types.ts files in which we make efforts to describe accurate __typenames based on the schema, wondering if we should always assume that for draft entities, __typename will be omitted?

[tdeekens]

Thanks. This is really not ideal! Wondering if instead of leaking a "magic" boolean into the root type of model, we could add a removeFields to any draft model here. It's a bit more work but then better located maybe?

[ByronDWall]

Thanks. This is really not ideal! Wondering if instead of leaking a "magic" boolean into the root type of model, we could add a removeFields to any draft model here. It's a bit more work but then better located maybe?

@tdeekens Im certainly not against it, but I will have to look into how that would work a little more to understand it completely from my end

[tdeekens]

Thanks. I'd value that. We'd mainly add a removeFields to every transformer of each draft type in the graphql transformer. It's more repetitive but should work to my memory.

[ByronDWall]

Thanks. I'd value that. We'd mainly add a removeFields to every transformer of each draft type in the graphql transformer. It's more repetitive but should work to my memory.

@tdeekens We already have a follow-up pr planned that will update types for drafts and do much of what you are asking, but the approach you are suggesting will not remove __typename values from nested transformed fields (eg a productType reference in products) since fieldsToRemove runs a delete only on fields that match in the top-level object.

Deleting nested fields with removeFields seems like it could introduce unwanted and unexpected behavior. Which, thinking back on the original draft PR that I originally put up a couple months ago, this is a large reason why I went with making a specific "magic" boolean option on the root transformer type, since graphql drafts are a bit of a different usecase.

[jaikamat]

@tdeekens This makes me wonder if there's value in typing __typename for drafts and creating it at all? Should we remove all __typename declarations from drafts in types.ts and not add them in the GraphQL transforms, for draft entities?

[tdeekens]

@tdeekens This makes me wonder if there's value in typing __typename for drafts and creating it at all? Should we remove all __typename declarations from drafts in types.ts and not add them in the GraphQL transforms, for draft entities?

Yes. That would be another more native option to test-data. If we prevent it from being added at all that solve the root cause nicely.

[jaikamat]

@tdeekens This makes me wonder if there's value in typing __typename for drafts and creating it at all? Should we remove all __typename declarations from drafts in types.ts and not add them in the GraphQL transforms, for draft entities?

Yes. That would be another more native option to test-data. If we prevent it from being added at all that solve the root cause nicely.

Honest question - if we set the expectation that draft GraphQL entities never have __typename built, can we carry this over to non-draft models? This means that consumers won't have the ability to assert against __typename in testing, but I wonder how problematic this is.

Seems to me that if we don't build __typename for draft models, then they're already not GraphQL schema-compliant.

We could definitely audit instances of __typename, remove them, and greatly simplify some of these issues (especially for dual use-case models like Reference where there is no draft model)

[tdeekens]

Honest question - if we set the expectation that draft GraphQL entities never have __typename built, can we carry this over to non-draft models? This means that consumers won't have the ability to assert against __typename in testing, but I wonder how problematic this is.

That won't work. It would be needed the latest for Apollo and it's client-side cache. So there, given we mock with msw, the typename is needed.

Seems to me that if we don't build __typename for draft models, then they're already not GraphQL schema-compliant.

Overall this is the crux. As per GraphQL spec __typename is only used for Object, Interface, or Union. Not an Input type - what is passed with a mutation.

If an Model was only used for an input type in a GraphQL schema. Then we could safely remove it. If however, drafts are also used for "unsaved" entities there is an issue. As in that case it would be a mocked response and flow into e.g Apollo's cache.

If drafts are used as responses to GraphQL queries?

Does this make sense?

[jaikamat]

Overall this is the crux. As per GraphQL spec __typename is only used for Object, Interface, or Union. Not an Input type - what is passed with a mutation.

If an Model was only used for an input type in a GraphQL schema. Then we could safely remove it. If however, drafts are also used for "unsaved" entities there is an issue. As in that case it would be a mocked response and flow into e.g Apollo's cache.

If drafts are used as responses to GraphQL queries?

Does this make sense?

Toooootally makes sense now given that context, as draft models are effectively input types. So seems like the path forward is to create draft models for all "dual-use-case" models (like Reference), use them where necessary, and never specify a __typename to be built?

[tdeekens]

Toooootally makes sense now given that context, as draft models are effectively input types. So seems like the path forward is to create draft models for all "dual-use-case" models (like Reference), use them where necessary, and never specify a __typename to be built?

Yes to my current understanding. For context: we had a time in which a draft was a preset of an entity in which we unset the __typename using eg .__typename(undefined). Still as drafts and regular entities at times are structurally too different we decided to split them in two models. Given that, and the clear input oriented use case of drafts, we might just never specify a typename to be build.

Unless of course there are other valid and good reasons to keep it.

P.S.: Lets add ADRs to this repo to document decisions.