fix(visitor-plugin-common): avoid reading from null values

[frandiox]

Description

tl;dr visitor-plugin-common tries to read from a variable that can be null in some situations. It's probably hard to reproduce since I'm using custom stuff. However, the change in this PR is just making the code more robust by moving 1 line that reads from a variable after an existing null check.

---

While working on codegen for Shopify/Hydrogen, I found that the following fragment breaks when using typescript-operation in @graphql-codegen/cli v4 and v5. It used to work in v3.3.1:

const CART_QUERY_FRAGMENT = `#graphql
  fragment CartLine on CartLine {
    id
    quantity
  }
  fragment CartApiQuery on Cart {
    id
    lines {
      nodes {
        ...CartLine
      }
    }
  }
`

It throws with [Codegen] Cannot read properties of null (reading 'union'), which comes from this line:

const hasUnions = grouped[typeName].filter(s => typeof s !== 'string' && s.union).length > 0;

In the code above, grouped contains the following data:

{
  grouped: {
    CartLine: [ "Pick<StorefrontAPI.CartLine, 'id' | 'quantity'>" ],
    ComponentizableCartLine: [ null ]
  }
}

The fragment for CartLine is correct, but the selection set for "nodes {...}" (ComponentizableCartLine here) has a null value which probably wasn't there in 3.3.1. Even though the type says it should be a string, truth is it can also be null because that's what is returned here.

It works when adding anything next to the fragment spread (the null value becomes a string):

const CART_QUERY_FRAGMENT = `#graphql
  fragment CartLine on CartLine {
    id
    quantity
  }
  fragment CartApiQuery on Cart {
    id
    lines {
      nodes {
        id # <=== anything here makes it work
        ...CartLine
      }
    }
  }
`

I'm not sure what changed since 3.3.1, perhaps it's the type of data that comes from other packages different from visitor-plugin-common, but I think it's not bad to make this package more robust and filter out null values.

The change in this PR is simply moving the offending line a few lines below after an existing check for null values.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Quite a manual test, but the code change is just making it more robust so it can't break anything else.

Test Environment:

• OS: macOS Ventura 13.5.2
• @graphql-codegen/...: ^5
• NodeJS: 18.12.0

Checklist:

• I have followed the CONTRIBUTING doc and the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[changeset-bot]

🦋 Changeset detected

Latest commit: 51798c1

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

This PR includes changesets to release 9 packages

Name	Type
@graphql-codegen/visitor-plugin-common	Patch
@graphql-codegen/typescript-document-nodes	Patch
@graphql-codegen/gql-tag-operations	Patch
@graphql-codegen/typescript-operations	Patch
@graphql-codegen/typescript-resolvers	Patch
@graphql-codegen/typed-document-node	Patch
@graphql-codegen/typescript	Patch
@graphql-codegen/graphql-modules-preset	Patch
@graphql-codegen/client-preset	Patch

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

[frandiox]

relevant.length === 0 will be true for [null] so it exits before declaring hasUnions.

[frandiox]

Tests are passing but pr / algolia / algolia-records-check (pull_request) is failing. I see it also fails in other unrelated PRs so I'm assuming it's something broken upstream 🤔

[saihaj]

Can you please add a failing test case?

[saihaj]

Tests are passing but pr / algolia / algolia-records-check (pull_request) is failing

PR algolia breaks on forks because it needs write access. I wouldn’t worry about this

[frandiox]

I think I've found the combination that makes it fail. It works when you do at least one of these:

• In the schema file, remove the whole ComponentizableCartLine type.
• Turn off either skipTypename or mergeFragmentTypes.
• Add id or quantity in the selection field next to the ...CartLine in the query file.

When this whole situation happens, the hasUnions check fails because the array is [null]. It used to be [] in this same situation in graphql-cli@3.3.1.

I don't have enough context to understand why this array now contains null, just that making this piece of code more robust makes it work again. Maybe this information will give you a hint on how to find the reason why null is added 🤞

[frandiox]

For some reason, yarn examples:build is failing because swc_plugin.wasm is missing from @graphql-codegen/client-preset-swc-plugin. This seems to be happening in master branch as well so I guess it's unrelated to this PR.

[saihaj]

thank you!