[Breaking]: architectural change to omit draft model __typename creation

[jaikamat]

Summary

This PR makes sweeping changes to draft models, and introduces a proposed ADR we can agree on going forward.

The problem

Note that the changes in this PR only apply to draft models, not base models.

While __typename is technically part of the GraphQL schema for draft/input types, using it in practice has proven challenging. When we build draft GraphQL models and attempt to directly submit them as variables via the CoCo API, we are met with 400 errors, because we build __typename on many draft models internally.

This has led to our team having to shim omitDeep functions in our sample data creation resolvers to strip __typename, and casting to appropriate types, which is neither type safe nor ideal. I suspect that some consumers using draft models will have to do the same.

Proposed solution

@ByronDWall had done great work trying to solve this here, however that raised questions about the very purpose of draft models.

We learned that models were originally designed without drafts, but as testing necessitated mocking requests, drafts were split out to represent input types (and as many of you know, GraphQL drafts may deviate wildly from their base REST representations, so splitting them out proved very maintainable).

Therefore, we're setting the precedent here that draft GraphQL models should adhere to their preferred utilitarian shape (which excludes __typename), rather than their strict schema representation. After all, this is a test data repository, and our form should follow function.

There was one lingering problem with this: Reference, KeyReference, and LocalizedString did not have draft representations, and always included __typename when built for GraphQL. They essentially had a "dual use case". If we allowed them to be built as-is, our live API requests would still fail.

Therefore, they have been extended to include their respective *Draft representations (which exclude __typename), and all points of implementation in draft models have been edited to use them.

Further, we have removed __typename from those draft models that built it.

Tests and snapshots have been updated accordingly.

TL;DR

Draft GraphQL models no longer have __typename. Please indicate if this will have downstream complications for your team. We are considering this a breaking change

[changeset-bot]

🦋 Changeset detected

Latest commit: 483b152

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

This PR includes changesets to release 34 packages

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

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]

@commercetools/first-contact-team-fe No draft preset snapshot tests contain __typename

[jaikamat]

For most of the models in this PR (save for Reference, KeyReference, and LocalizedString), this was the relevant change. Remove __typename from types.ts, the generator, and ensure the type was safe all the way down to tests

[jaikamat]

Highlighting the ADR here for others to review 🔍

[tdeekens]

🏩 The changes look very sane. I value taking the time taken for this approach! Especially the ADR addition.

The main question I guess now: breaking or not. I would rather be safe and make it a breaking release. WDYT? Consumers can still add a typename to drafts by adding __typename('foo') I suppose?

[jaikamat]

🏩 The changes look very sane. I value taking the time taken for this approach! Especially the ADR addition.

The main question I guess now: breaking or not. I would rather be safe and make it a breaking release. WDYT? Consumers can still add a typename to drafts by adding __typename('foo') I suppose?

Definitely comfortable labeling this as a breaking release considering the number of presets and files it touches. How do I go about doing that?

To be honest consumers will likely have some difficulty tacking __typename on, especially for deeply-nested models, but that's the tradeoff. If teams are relying on __typename in draft GraphQL model tests, I'd be curious to see how widespread of an issue it is - but my suspicions tell me it's minimal, if at all.

[emmenko]

Approving the approach, the documentation and the effort put into this! Many thanks ❤️

I won't be reviewing every single change though, I trust your abilities 😉

[jaikamat]

@tdeekens see cc0edf3 for the changeset 👍

[tdeekens]

Super duper. So proud how far test-data came from a small idea to things falling into place.

[stephsprinkle]

This is a huge improvement 👏

[jaikumar-tj]

👏 Thanks for such a massive work

[kterry1]

Awesome job Jai!

[tdeekens]