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

TypeScript performance regression with the MaybeMasked feature. #12248

Open
paales opened this issue Dec 30, 2024 · 3 comments · May be fixed by #12252
Open

TypeScript performance regression with the MaybeMasked feature. #12248

paales opened this issue Dec 30, 2024 · 3 comments · May be fixed by #12252
Labels
🎭 data-masking Issues/PRs related to data masking

Comments

@paales
Copy link

paales commented Dec 30, 2024

Issue Description

{
  "scripts": {
    "tsc:perf": "NODE_OPTIONS=--max_old_space_size=10000 tsc --noEmit --generateTrace tsctrace --incremental false && npx @typescript/analyze-trace tsctrace",
  },
}

With Apollo Client 3.11.10, my only hotspot is due to using MUI (so unrelated)

time yarn tsc:perf
Hot Spots
└─ Check file /users/paulhachmang/sites/graphcommerce/examples/magento-graphcms/components/theme.ts (1470ms)

yarn tsc:perf  38,32s user 4,69s system 129% cpu 33,138 total

With Apollo Client 3.12.4 it shows files varioius files that use some derivative of useQuery:

time yarn tsc:perf
Hot Spots
├─ Check file /users/paulhachmang/sites/graphcommerce/examples/magento-graphcms/components/theme.ts (1387ms)

├─ Check file /users/paulhachmang/sites/graphcommerce/examples/magento-graphcms/pages/cart.tsx (1297ms)
│  └─ Check variable declaration from (line 39, char 9) to (line 39, char 31) (1254ms)
├─ Check file /users/paulhachmang/sites/graphcommerce/packages/google-datalayer/plugins/googledatalayeruseremoveitemfromcart.tsx (1017ms)
│  └─ Check deferred node from (line 21, char 5) to (line 46, char 7) (1008ms)
│     └─ Check expression from (line 22, char 7) to (line 45, char 9) (1008ms)
│        └─ Check expression from (line 24, char 19) to (line 45, char 9) (1008ms)
│           └─ Check expression from (line 24, char 20) to (line 45, char 8) (1008ms)
│              └─ Check expression from (line 26, char 21) to (line 44, char 10) (1008ms)
├─ Check file /users/paulhachmang/sites/graphcommerce/packages/magento-product/components/addproductstocart/useaddproductstocartaction.ts (1012ms)
│  └─ Check variable declaration from (line 28, char 9) to (line 29, char 31) (992ms)
├─ Check file /users/paulhachmang/sites/graphcommerce/packages/google-datalayer/plugins/googledatalayerremoveitemfromcart.tsx (957ms)
│  └─ Check variable declaration from (line 11, char 14) to (line 28, char 2) (955ms)
│     └─ Check expression from (line 11, char 88) to (line 28, char 2) (954ms)
│        └─ Check expression from (line 16, char 10) to (line 27, char 5) (954ms)
│           └─ Check expression from (line 16, char 18) to (line 27, char 4) (951ms)
│              └─ Check expression from (line 18, char 17) to (line 26, char 6) (951ms)
├─ Check file /users/paulhachmang/sites/graphcommerce/examples/magento-graphcms/pages/checkout/payment.tsx (818ms)
│  └─ Check variable declaration from (line 47, char 9) to (line 48, char 101) (801ms)
│     └─ Check expression from (line 48, char 5) to (line 48, char 101) (801ms)
│        └─ Check expression from (line 48, char 5) to (line 48, char 34) (801ms)
│           └─ Check expression from (line 48, char 12) to (line 48, char 34) (801ms)
│              └─ Check expression from (line 48, char 12) to (line 48, char 28) (801ms)
├─ Check file /users/paulhachmang/sites/graphcommerce/packages/magento-cart/components/cartitemsummary/cartitemsummary.tsx (779ms)
│  └─ Check variable declaration from (line 43, char 9) to (line 43, char 88) (748ms)
└─ Check file /users/paulhachmang/sites/graphcommerce/examples/magento-graphcms/pages/p/[url].tsx (620ms)
   └─ Check variable declaration from (line 68, char 9) to (line 68, char 56) (444ms)
      └─ Check expression from (line 68, char 40) to (line 68, char 56) (444ms)

No duplicate packages found
yarn tsc:perf  70,13s user 9,66s system 125% cpu 1:03,80 total

An example of a used derivative:

/**
 * Requires the query to have a `$cartId: String!` argument. It will automatically inject the
 * currently active cart_id.
 *
 * Example:
 *
 * ```tsx
 * const { data } = useCartQuery(CartFabQueryDocument)
 * ```
 */
export function useCartQuery<Q, V extends { cartId: string; [index: string]: any }>(
  document: TypedDocumentNode<Q, V>,
  options: QueryHookOptions<NoInfer<Q>, NoInfer<Omit<V, 'cartId'>>> = {},
): QueryResult<Q, V> {
  const { ...queryOptions } = options

  const router = useRouter()
  const { currentCartId, locked } = useCurrentCartId()

  const urlCartId = router.query.cart_id
  const usingUrl = typeof urlCartId === 'string'
  const cartId = usingUrl ? urlCartId : currentCartId
  const shouldLoginToContinue = useCartShouldLoginToContinue()

  if (usingUrl || locked) queryOptions.fetchPolicy = 'cache-only'

  if (usingUrl && typeof queryOptions.returnPartialData === 'undefined')
    queryOptions.returnPartialData = true

  queryOptions.variables = { cartId, ...options?.variables } as V

  const query = useQuery(document, {
    ...(queryOptions as QueryHookOptions<Q, V>),
    skip: queryOptions.skip || !cartId || shouldLoginToContinue,
  })

  if (shouldLoginToContinue && !queryOptions?.skip) {
    return {
      ...query,
      error: new ApolloError({
        graphQLErrors: [
          new GraphQLError('Action can not be performed by the current user', {
            extensions: { category: 'graphql-authorization' },
          }),
        ],
      }),
    }
  }

  return query
}

Link to Reproduction

https://github.com/graphcommerce-org/graphcommerce

Reproduction Steps

Same codebase but with the more version of @apollo/client causes the regression.

@apollo/client version

3.12.4

@paales
Copy link
Author

paales commented Jan 6, 2025

Our current implementation didn't use any data masking so we didn't need to 'disable' the data-masking feature we can just have it enabled.

declare module '@apollo/client' {
  interface DataMasking {
    enabled: true
  }
}

Adding this solved our issue.

@paales paales closed this as completed Jan 6, 2025
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

@jerelmiller
Copy link
Member

@paales thanks so much for bringing this to our attention. @phryneas and I discussed this morning that we'd like to change the default to leave types unchanged unless opting into data masking or opting out of it as well. I'm going to go ahead and reopen this issue so we can track this along with the PR.

@jerelmiller jerelmiller reopened this Jan 6, 2025
@jerelmiller jerelmiller added the 🎭 data-masking Issues/PRs related to data masking label Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎭 data-masking Issues/PRs related to data masking
Projects
None yet
2 participants