Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(core): Deprecate maskTypename option #3299

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

kitten
Copy link
Member

@kitten kitten commented Jun 29, 2023

See: #3297 (comment)

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:

  • the API mimicks inputs that mirror outputs
  • the user doesn't actually provide the inputs
  • the selection sets of both match precisely

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

  • Deprecate maskTypename option and function

@kitten kitten merged commit cbf338b into main Jun 30, 2023
@kitten kitten deleted the chore/deprecate-mask-typename branch June 30, 2023 11:25
@github-actions github-actions bot mentioned this pull request Jun 30, 2023
@yurks
Copy link
Contributor

yurks commented Jun 6, 2024

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

@kitten
Copy link
Member Author

kitten commented Jun 6, 2024

@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
Copy link
Contributor

yurks commented Jun 6, 2024

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

@kitten
Copy link
Member Author

kitten commented Jun 6, 2024

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
Copy link

pReya commented Jun 27, 2024

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
Copy link
Member Author

kitten commented Jun 27, 2024

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
Copy link

pReya commented Jun 28, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants