0no-co / GraphQLSP

TypeScript LSP plugin that finds GraphQL documents in your code and provides diagnostics, auto-complete and hover-information.
https://gql-tada.0no.co
MIT License
332 stars 13 forks source link

Unused co-located fragment definition(s) false-negatives? #316

Closed benjamindulau closed 2 months ago

benjamindulau commented 2 months ago

I get a ton of "Unused co-located fragment definition(s)" warnings but I don't understand why...

For instance, I get this warning:

Capture d’écran 2024-05-06 à 12 06 19

Even though I do use the fragment in this very same file as you can see further in the same file code:

Capture d’écran 2024-05-06 à 12 06 45

Why is that?

JoviDeCroock commented 2 months ago

Are you exporting that fragment? Basically if you follow the principles of fragment co-location an exported fragment imposes a data requirement on the importer so it gives the expectation that the importer uses this fragment in its query.

https://gql-tada.0no.co/guides/fragment-colocation

You can turn this setting off if you are not following these principles

benjamindulau commented 2 months ago

Well, we use Remix (React Router), so this is a route component with a loader function which sends the GQL queries with imported fragments from child components and then renders each of those child component with the resulting data as props. Each child then reads its fragment from the resulting data (fragment masking), etc.

Something like:

// components/ProgrammeHeader.tsx
export const ProgrammeHeaderFragment = graphql(
  `
    fragment ProgrammeHeaderFragment on Programme {
      id
      name
    }
  `,
);

export type ProgrammeHeaderFragmentType = FragmentOf<
  typeof ProgrammeHeaderFragment
>;

export default function ProgrammeHeader({ data }: {  data: ProgrammeHeaderFragmentType; }) {
  const header = readFragment(ProgrammeHeaderFragment, data);

  return (
    <h1>Whatever {header.name}</h1>
  );
}
import ProgrammeHeader, {
  ProgrammeHeaderFragment,
  ProgrammeHeaderSkeleton,
} from "./components/ProgrammeHeader"; // <= Warning: Unused co-located fragment definition(s) "ProgrammeHeaderFragment" in "./components/ProgrammeHeader"

// route.tsx
export async function loader({ request, context, params }: LoaderFunctionArgs) {
  invariant(params.slug, "Slug not defined");
  const slug = params.slug;

  const programmeHeaderPromise = context.apollo
    .query({
      query: graphql(
        `
          query GetProgramme($slug: String!) {
            programme(slug: $slug) {
              ...ProgrammeHeaderFragment
            }
          }
        `,
        [ProgrammeHeaderFragment],
      ),
      variables: {
        slug,
      },
    })
    .then(({ programme }) => {
      if (!programme) {
        throw notFound("Programme not found", request);
      }

      return programme;
    });

    return defer({
      programmeHeaderPromise,
    });
}

export default function ProgrammePage() {
  const { programmeHeaderPromise } = useLoaderData<typeof loader>();

  return (
    <>
      <Suspense fallback={<ProgrammeHeaderSkeleton />}>
          <Await resolve={programmeHeaderPromise}>
            {(data) => (
              <ProgrammeHeader data={data} />
            )}
          </Await>
        </Suspense>
      {/*...*/}
    </>
  );
}

I mean the previous code is a very common use case.

I agree that the "fragment" itself is not "really used" directly in the route component, but it is in the GraphQL query and I think the developer shouldn't be warned about it being unused here. Perfectly fine usage of fragments colocation I think.

Do you mean we should read the fragment before passing it to the child component and not in the child itself? I won't go into details but that creates other edge cases.

I don't want to disable the feature entirely because we still want to warn the developer when his fragment is really not used.

JoviDeCroock commented 2 months ago

Oh if it's warning you for that second example then we probably have a missing case in our search because we should find

      query: graphql(
        `
          query GetProgramme($slug: String!) {
            programme(slug: $slug) {
              ...ProgrammeHeaderFragment
            }
          }
        `,
        [ProgrammeHeaderFragment],
      ),

Which clearly uses this fragment, the logic that needs debugging can be found here. If you're not up for debugging this I'd love for a reproduction!

benjamindulau commented 2 months ago

I did a reproduction here https://stackblitz.com/edit/remix-run-remix-fh1a6h?file=app%2Froutes%2Fprogramme%2Froute.tsx

I don't know why Stackblitz doesn't show the warning in the editor directly, but if you run npx gql.tada check you can see the warning.

Capture d’écran 2024-05-07 à 10 03 43

JoviDeCroock commented 2 months ago

Basically, it starts working when you extract the graphql() call to the global scope, this is because we don't traverse nested call expressions at this point in time. We could make this work by adding it here however then we'd also have to find a way to avoid traversing deeply into other files/... which is a performance concern we could address by checking whether the callExpression contains nested callExpressions for instance.

benjamindulau commented 1 month ago

@JoviDeCroock I just updated to the last version and it fixed a lot of warnings!

But it seems that there are still some issues when only "some" of the fragments of an import are used.

For instance, the following component exports two different fragments. Each of them can be imported and used from different files: Capture d’écran 2024-05-29 à 10 25 59

Only one them (fragment) is imported/used in this file: Capture d’écran 2024-05-29 à 10 18 28

The other fragment is imported and used in another file: Capture d’écran 2024-05-29 à 10 23 26

You can see the issue in the warnings messages.

JoviDeCroock commented 1 month ago

@benjamindulau those are correct warnings though, as I did before, I encourage you to read up on fragment co-location or disable the functionality