RFC: A type-safe readResult helper, to make writing tests easier

[louy]

Summary

Tada is an awesome library, solves almost all the pain points of codegen preset-client, except one: writing test fixtures with fragment masking turned on.

Currently, when writing a test fixture, you have two options:

1. Use the unsafe_readResult function, which treats fragments as type any and hence is unsafe.
2. Mask each individual fragment explicitly, causing the test fixture to be entangled with the implementation of the query & hierarchy of fragments (and thus UI components).

Here's a simplified example from our code base for case 2:

import { graphql, maskFragments, ResultOf } from 'gql.tada';

export const FavouritesCardFragment = graphql(`
  fragment FavouritesCardFragment on Location {
    id
    name
  }
`);
const FavouritesQuery = graphql(
  `
    query Favourites {
      favouritedLocations {
        edges {
          node {
            id
            ...FavouritesCardFragment
          }
        }
      }
    }
  `,
  [FavouritesCardFragment],
);
const mockedFavouritesData: ResultOf<typeof FavouritesQuery> = {
  favouritedLocations: {
    edges: [
      {
        id: '1',
        name: 'Location A',
      },
      {
        id: '2',
        name: 'Location B',
      },
    ].map(node => {
      return {
        node: {
          id: node.id,
          ...maskFragments([FavouritesCardFragment], node),
        },
      };
    }),
  },
};
console.log(mockedFavouritesData);

Each time any of the internals of query Favourites are changed, such as which fragments it uses or which components they come from, our test fixtures also need to change, despite the actual behaviour (how the server sees it) and thus the fixture itself remaining unchanged.

Proposed Solution

In theory, unsafe_readResult doesn't need to be unsafe. However, the problem right now is that TadaDocumentNode does not keep references to the fragments that were passed into the GraphQLTadaAPI call signature. If the list of fragments was to be retained somehow, then readResult (safe version of unsafe_readResult) can traverse the ResultOf<Document> type and instead of using omitFragmentRefsRec<ResultOf<Document>> these refs can be looked up in the fragments list and added back to the result type.

In a very simplified way:

type getDocumentNode<
  Document extends DocumentNodeLike,
  Introspection extends SchemaLike,
  Fragments extends {
    [name: string]: any;
  } = {},
  isMaskingDisabled = false,
 > = getDocumentType<Document, Introspection, Fragments> extends never
  ? never
  : TadaDocumentNode<
      getDocumentType<Document, Introspection, Fragments>,
      getVariablesType<Document, Introspection>,
-     decorateFragmentDef<Document, isMaskingDisabled>
+     decorateFragmentDef<Document, isMaskingDisabled>,
+     Fragments
    >;

interface TadaDocumentNode<
  Result = {
    [key: string]: any;
  },
  Variables = {
    [key: string]: any;
  },
  Decoration = void,
+ Fragments extends {
+   [name: string]: any;
+ } = {},
> extends DocumentNode,
    DocumentDecoration<Result, Variables>,
-   DefinitionDecoration<Decoration> {}
+   DefinitionDecoration<Decoration> {
+     __fragments: Fragments,
+   }

And then a safe readResult that uses something similar to omitFragmentRefsRec can be used.

Requirements

• TadaDocumentNode retains information about what fragments were used/masked.
• A new readResult function is introduced, that is type safe (retains type information about what's been masked by fragment masking)

[kitten]

This is on my radar, but to be honest, I'm not a fan of this approach and am rather trying to think of alternative APIs that make composing results easier.

If the list of fragments was to be retained somehow, then readResult (safe version of unsafe_readResult) can traverse the ResultOf type and instead of using omitFragmentRefsRec<ResultOf> these refs can be looked up in the fragments list and added back to the result type.

This is imho not acceptable and will lead to a degredation of TypeScript performance. While this is somewhat acceptable with disableMasking, passing through all fragments leads to:

• more noise in the type output
• potentially fewer lazy evaluations by TS (and hence a negative effect on all TS features)
• even if all fragments are available there, they'd still have to be unwrapped in a second step

I can imagine a type that recursively unwraps fragments. This is potentially just as costly but isolated to a usage site in the tests, similar to maskFragments(), but on demand.

That said, I still don't 100% agree with that API, because this assumes that we want to have exact result types as test fixtures, which isn't always the case. I basically am not convinced that it's ergonomic and nice to create huge mock results in this manner, and would still recommend an unsafe cast for that, to encourage these data fixtures to not be handwritten (i.e. generated from real data)

This also kind of leads into another point, testing in the presence of GraphQL. When you use co-located fragments, I find that mock tests that give fake data to screens and render them to be hugely unhelpful over rendering the actual app with a mock API / staging API.

I can see the need for component testing in certain cases, and I know that people aren't always stringent with separating structural from presentational components, hence maskFragmetns exists for these cases.

But mocking whole queries while trying to keep types safe could, with an API that encourages hand-written data, be an overall worse experience 🤔 Not saying the idea is entirely meritless, but I'm also not a huge fan of a type-safe readResult in terms of effort vs payoff

[louy]

I agree with you, especially about test fixtures. Most of the time you don't need 100% of the result type for a test fixture, but this isn't something that can't be solved with Partial or RecursivePartial type.

To address your points:

• I don't see much noise that's unmanageable by adding fragments list to TadaDocumentNode as a type argument. That type is almost never used on its own, and the suggestion here would not affect ResultsOf and VariablesOf, unless you had something else in mind?
• I'm not really familiar with lazy evaluation in typescript, and couldn't find much when I looked it up. Can you elaborate here or share any links to relevant resources?
• Yes and that's unescapable, but still solves the problem of having tests need to reflect implementation of which fragments are used in which query

I really dislike the idea of having manually-coded fixtures in general, but there is another more acceptable use-case than the one I mentioned: components that use other components (fragments referencing other fragments), and need manual fixtures such as for storybooks.

const VenueCardFragment = graphql(`
fragment VenueCardFragment on Venue {
  id name image
}
`);
const VenueCard: React.FC<{venue: ResultOf<typeof VenueCardFragment>}> = ({venue}) => { /* ... */ }

const VenuesCarouselFragment = graphql(`
fragment VenuesCarouselFragment on VenuesConnection {
  edges { node { ...VenueCardFragment } }
  pageInfo { hasNext nextCursor }
}
`, [VenuesCarouselFragment]);
/** A carousel of venues with pagination/infinite scroll etc */
const VenuesCarousel: React.FC<{data: ResultOf<typeof VenuesCarouselFragment>}> = ({data}) => { /* ... */ }

// Story/component tests of VenuesCarousel will need to unmask and know about VenuesCarouselFragment, even though we're still at the component/fragment level

The problem with splitting components between presentational & structural is it either encourages immature abstractions, where people start creating very generic components too early, or it gets too verbose, where people have to re-declare the query return type as a props type in their presentational components to avoid coming up with a new abstraction too early.

I am happy to see any other proposals you might have that solve the main requirement here really: users have the ability to get the unmasked return type from the return value of graphql() function, without needing to re-declare what was passed into that function (i.e. fragments list).

[louy]

Just to emphasis: I've been pushing my teams to start adopting fragment co-location, and after months of playing with the idea we found that tada is the only tool that allows incremental adoption with decent DX, so really appreciate the work that was done here. Just trying to make that adoption barrier lower for my teams and the community overall.

[JoviDeCroock]

I think personally I am in a similar boat to Phil, when we talk about unit-testing I like to think that we test a component in isolation and not the children/... I do realise that this leans towards a different discussion in terms of the whole shallow vs non-shallow rendering during tests. I do however believe that this is a similar problem space, we co-locate our fragments so we don't have to care in the parent-component about what data is needed by the children.

I'm not really familiar with lazy evaluation in typescript, and couldn't find much when I looked it up. Can you elaborate here or share any links to relevant resources?

There are sadly very little resources on this but the gist of it is that TypeScript as it evaluates types will often try to defer work to when it's needed/used/... This means that de-normalizing a type like the Fragment by immediately including it will force TS to evaluate it and hence incur more work. This is a pretty big cost to make testing easier imho.

I do see the case for storybook and visual regression testing there, we have faced something similar where I work. We do however use the approach you describe initially where we keep calling maskFragments. My thinking is more going into the direction of an alternative helper that doesn't change the core types as we worked a lot on performance of this library to be acceptable.

[kitten]

Just bumping this here 😄 I know, unusual for a maintainer to bump a feature request.

I want to basically lay out some requirements, if anyone has an alternative proposal for maskFragments and unsafe_readResult (or improvements) that:

• improve the ability to build up mock result data
• ensure that fragment masking is still enforced (somewhat)
• fix the problem of mocked data and/or fake data specifically (including for cache updaters)
• and; does not break internal type separation (i.e. doesn't imply that @_unmask is used everywhere and doesn't pass on fragment types on each type, which would worsen TS laziness and performance)

Then we're happy to look into this. From our end, we currently don't have any proposals or solutions that improve this. Sorry! 😅

[louy]

I haven't looked at your codebase since opening the ticket above, so apologies if what I'm saying makes no sense, but what if every document node came with two result types: Masked and Unmasked

The unmasked type would be generated identically to the masked type, if the masking option was turned off

Then you can have a utility UnmaskedResultOf type that can extract that

The unmasked result type can be exposed either as a property on the document (similar to how I was proposing exposing __fragments) or you can store it on some interface type by giving each doc an id and augmenting that interface (similar to your cache/turbo type I believe)