urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.6k stars 448 forks source link

chore(core): Deprecate maskTypename option #3299

Closed kitten closed 1 year ago

kitten commented 1 year ago

See: https://github.com/urql-graphql/urql/issues/3297#issuecomment-1612987757

Summary

This PR, when applied, deprecates maskTypename (both the option and the utility). The option may be removed sooner than the utility function.

In short, applying maskTypename was not recommended and applying it in an exchange yourself is still supported in the future. This is because it's often only used (and requested) by users given that they want to pass GraphQL objects back into their mutations as GraphQL input objects, which requires the following pre-conditions to apply:

However, compared to Graphcache's output for example, it leads to non-faithful data shapes, and is only used to “save” time and not map to input objects, which is a short task.

Set of changes

yurks commented 3 months ago

We are using maskTypename for some mutations to not trigger cache updates. What is a recommended way to do that after this deprecation?

kitten commented 3 months ago

@yurks You can copy and use the utility as it was manually, or construct the mutation inputs. We don't want to encourage GraphQL outputs and inputs to be made to match, since that's — at least with fragment masking — extremely unlikely to happen naturally. If it's done on purpose it is a constraint that quickly takes a lot of benefits out of GraphQL's predictability when things go wrong, in terms of being able to modify selection sets quickly and independently from mutation inputs.

So, overall, if you do need it for migration purposes, try replicating the utility in your own codebase. Otherwise, try creating creation/transform utilities that return the input type(s) you'll need for your mutations.

yurks commented 3 months ago

thanks, are there any examples how to implement "creation/transform utilities that return the input type(s) you'll need for your mutations"?

kitten commented 3 months ago

I mean, you just take your inputs and return the output object is what I meant. Instead of striving to pass the same output objects as input objects, to create functions that create the input

pReya commented 2 months ago

We don't want to encourage GraphQL outputs and inputs to be made to match, since that's — at least with fragment masking — extremely unlikely to happen naturally

@kitten Do you mind extending on our answer here? Why is this unlikely? How is this connected to Fragment masking? Would appreciate a "newbie" answer here.

kitten commented 2 months ago

Say, you have a GraphQL object and input that look like this:

type Author {
  id: ID!
  name: String!
}

input UpdateAuthor {
  id: ID!
  name: String!
}

And a corresponding mutation that looks like this:

type Mutation {
  updateAuthor(input: UpdateAuthor!): Author!
}

[!NOTE] Let's ignore here that id isn't a separate argument on updateAuthor, which is a common pattern, just to make this example work with the above, where the intention is that UpdateAuthor matches the shape of Author.

and this is requested as part of an AuthorOverview component. This component may include the author, in this example with a colocated fragment, as such:

fragment AuthorOverview on Author {
  id
  name
}

The component also has a button that opens a section using which someone can update this author. Let's say, because this is a sub-component it now uses a similar or the same fragment and then has a mutation such as the following:

mutation UpdateAuthor($input: UpdateAuthor!) {
  updateAuthor(input: $input) {
    id
    ...AuthorOverview
  }
}

Because the shape of UpdateAuthor and Author mirror each other, let's say that the mutation receives { ...author, ...updatedAuthorData } as $input. This should, in theory, now match cases where people previously would've reached for maskTypename, since the only thing that doesn't match is __typename.

All is good so far

But, let's say, we now add a small sub-selection here. Say, the AuthorComponent loads a profile picture and we want this picture to change depending on an aspectRatio parameter. Let's extend the schema for that:

type ProfilePicture {
  id: ID!
  baseUrl: String!
  url: String!
  width: Int
  height: Int
}

type Author {
  id: ID!
  name: String!
  profilePicture(aspectRatio: Float): ProfilePicture
}

This is a bit contrived, but all that is important here is that we've added a non-primitive field.

We now extend the AuthorOverview fragment. Let's for the sake of this example not have a separate fragment just to illustrate the point.

fragment AuthorOverview on Author {
  id
  name
  profilePicture(aspectRatio: 1) {
    id
    url
  }
}

We now will get an error because we can't pass the above fragment's selection into the mutation any longer.

How could we fix this? How could we prevent this?

These are basically the same question. If we construct an UpdateAuthor input from scratch in the first place, or now switch this over, the issue is resolved, so instead of { ...author, ...updatedAuthorData }, we could create and pass:

const updateAuthorInput = {
  id: authorData.id,
  name: updatedData.name ?? authorData.name,
};

So, the whole premise of why maskTypename exists goes away when we construct inputs separately from how we receive selections.

The problem here fundamentally is that selections evolve separately. Even if a tool (such as gql.tada — small plug haha ❤️) issues a type error or other error when we pass an incorrect shape into our mutation, we can prevent this problem entirely if we don't connect our selections to our inputs.

On the API-side, they're separate types in our schema, so they may diverge. On the client-side, they're separately defined, so they may diverge. At any point in app development, fragment colocated or not, we may request different data and break our mutation, which isn't desireable.


A quick side-note on performance:

While this is solveable, it wasn't something we did want to do. Basically, maskTypename also defeats reference equality and hence can hurt re-rendering performance in most UI frameworks that have ways to leverage memo-optimisations (such as React.memo)

So, it can also be annoying to deal with maskTypename from a performance perspective, because it clones data and modifies it.

This of course could be solved with a small WeakMap cache. But since we were phasing this out this really wasn't worth it for us.


A note on why we removed this:

Ultimately, the decision to remove this comes from the main motivation that it a good feature: Because it's not a good feature we did remove this because it then just adds bloat if we keep it indefinitely.

The reason it's a "bad" feature is because it leads developers down the wrong path. It provides a solution to a problem that appears easier than the alternative. However, comparatively, given the above problems (and others) the end-result will lead to more pain and make fixing the problem harder.

I'd consider this feature a mistake today because it made doing the right thing less desireable and leads to many mutations breaking (if no checks are in place) unexpectedly, and makes the better solution appear harder than it really is.

Hope that makes sense ❤️


Can I still have this feature back?

Yes, you can still have this feature back if you copy its code. The code can be found here: https://github.com/urql-graphql/urql/blob/f14c94caeb8fdbda92a1c18bff414e210f174008/packages/core/src/utils/maskTypename.ts

And you can use a mapExchange to approximate what it did before, e.g.

import { mapExchange, cacheExchange, fetchExchange, Client } from '@urql/core';

new Client({
  url: '/graphql',
  exchanges: [
    mapExchange({
      onResult(result) {
        return result.operation.kind === 'query' 
          ? { ...result, data: maskTypename(result.data, true) }
          : result;
      },
    }),
    cacheExchange,
    fetchExchange,
  ],
})
pReya commented 2 months ago

@kitten Thank you very much for this detailled answer. This was very educational for me (and others, I suppose). Very much appreciated!