0no-co / gql.tada

🪄 Magical GraphQL query engine for TypeScript
https://gql-tada.0no.co
MIT License
2.52k stars 41 forks source link

GraphiQL VSCode warning: Undefined Fragment when spreading is used for an complex type #194

Closed alexdeia closed 4 months ago

alexdeia commented 4 months ago

Describe the bug

Hello. I faced problem with types when using fragment spreading.

For example:

If you have this query and want to use in another query your IDE won't discover as a type

export const ItemListFragment = graphql(`
  fragment PageInfo on ItemsConnection {
    pageInfo {
      hasNextPage
      hasPreviousPage
      startCursor
      endCursor
    }
  }
`)
export const ItemListQuery = graphql(`
  query GetItems () {
    items() {
      totalCount
      ...PageInfo
      nodes {
        ...SomeAnotherFragment - same problem
      }
    }
  }
`, [ItemListFragment])

IDE WebStorm error:

{
  nodes: {
    [$tada.fragmentRefs]
  :
    {
      ItemListItem: "Flight";
    }
    ;
  }
  [] | null;
  [$tada.fragmentRefs]
:
  {
    PageInfo: "Undefined Fragment";
  }
  ;
  totalCount: number;
}

But if you use directly the object with its props without the fragment, everything works

export const ItemListQuery = graphql(`
  query GetItems () {
    items() {
      totalCount
      pageInfo {
        hasNextPage
        hasPreviousPage
        startCursor
        endCursor
      }
      nodes {
        ...SomeAnotherFragment - same problem
      }
    }
  }
`, [ItemListFragment])

Reproduction

No response

gql.tada version

gql.tada: 1.4.3

Validations

JoviDeCroock commented 4 months ago

Hey,

With these sorts of issues we require you to create a reproduction, I also think your code fragments are either incorrect or out of date as you seem to allude to an issue being present with your description that that one works correctly 😅

But if you use directly the object with its props without the fragment, everything works

Then in the snippet ...SomeAnotherFragment - same problem

We can't really look at this and predict what your issue is as there isn't even remotely enough information nor does your description make sense.

Do note that there are also issues with JetBrains/Webstorm as they implement a custom TS LSP which doesn't have all the features. https://github.com/0no-co/gql.tada/issues/80

Also note that doing items() isn't valid GraphQL, not sure if you did this for brevity or that's real though.

alexdeia commented 4 months ago

OK, sorry, I'll try to prepare a real example.

echocrow commented 4 months ago

We ran into a similar problem in VSCode. Code executes as expected, but the IDE would complain about undefined fragments (when the fragment is defined in another variable and passed in as array in the second argument to graphql()) and unknown directives for e.g. @_unmask.

Turns out we not only had the Syntax Highlighting extension enabled, but also the official LSP extension. Once we disabled the LSP extension, typing for separately defined fragments works as expected. - Perhaps there is an incompatibility with the official GraphQL LSP extension that's worth calling out in the docs?

Long shot, but maybe OP is facing a similar issue in their IDE.

kitten commented 4 months ago

@echocrow: It's likely not an incompatibility, but rather, the LSP extension is issuing its own warnings that look very similar to ours, given that there's an overlap with the GraphiQL diagnostics that both output.

There's not really much we can do about that.

We can check whether a .vscode/extensions.json file is present in the repo, and whether that contains the plugin during the gql.tada doctor check. But some people may still have it installed for legitimate reasons, and other may have it installed without .vscode/extensions.json being present.

Do you have a sample of your graphql-config file? That's the only other thing we could check for, and I'd assume that'd have to be present for this to become an issue even.

echocrow commented 4 months ago

@kitten cheers for the response!

(this may be getting a bit off-topic from OP's post - unless OP was facing that exact issue too?)

Well, by "incompatibility" I meant that, as you described, the LSP extension would run its own checks and validation in the IDE. For most basic queries and mutations, that does not pose any issues. However, false errors do arise when using more "advanced" features, particularly around fragments. Those errors get flagged not by gql.tada or TS, but by the separate GraphQL LSP from the official extension. I assume these arise because that LSP does not recognize gql.tada's @_unmask directive, or imported fragments passed into queries via the fragments argument. These errors are safe to ignore once you know about this, but it certainly confused us initially when we evaluated gql.tada (and almost had us reconsider using it, thinking fragments weren't fully supported).

In an ideal world, maybe there's a way to either extend the extension LSP, helping it recognize gql.tada's extra features, or some way of instructing the GraphQL LSP to ignore gql.tada's queries function? I have no clue if either is possible though, as I know little to nothing about how that LSP works under the hood.

Short of a solution to have the two co-exist, that's why I suggested perhaps a note in the docs about this caveat (akin to the note about requiring VSCode to use the workspace TS version).

Do you have a sample of your graphql-config file?

Our exact setup is a little overly complex (monorepo and all). However, we're noticing this issue even with a relatively barebone graphql config file:

// .graphqlrc.ts
export default {
  projects: {
    foo: {
      schema: './packages/foo/schema.graphql',
      documents: './packages/foo/src/components/**/*.graphql',
    },
  },
}

For some reason, even this config has the GraphQL LSP validate e.g. packages/bar/my-gql-tada-query.ts (notice the unrelated foo vs bar directory). Perhaps this is actually a bug in the GraphQL LSP extension(?).

kitten commented 4 months ago

I just took a look, and this doesn't seem to be reproducible if the output is limited to: documents: './packages/foo/src/components/**/*.graphql', i.e. if the documents are limited to *.graphql files. Otherwise, it does seem like by default VSCode GraphQL (or rather graphql-language-service-server) will try to discover documents in *.{ts,tsx} files.

On a side-note, if your .graphqlrc.ts doesn't contain any project entries (or the root entry) that aren't limited to documents, then that's a bug. However, I didn't investigate this further since that'd require me to reproduce a larger config file and reproduction environment. it's possible that a schema entry on the root object of the config without a documents entry is sufficient to cause issues.

There's obviously still the question of whether it's possible to avoid the conflict entirely, and this is likely something I'd like to add to our doctor command, but I'm still looking into what we could do about this

kitten commented 4 months ago

There doesn't seem to be a better option here than to issue a warning to check that your documents setting in your graphql-config file only includes .graphql files, but that's basically the solution we'll have to go for for now.

echocrow commented 4 months ago

@kitten I'm pretty confident we ran into this issue even when documents was scoped to just .graphql files, or set to []. I can try to put together a repro later this week if that might be useful?

kitten commented 4 months ago

That'd be very helpful! 🙌 but potentially something that should also be reported to the graphql-config project itself.

Even if it turns out that, for some reason, documents isn't always fully respected, that's an issue for vscode-graphql and/or graphql-config.

There's also not a good setting in vscode-graphql that could be flipped in .vscode/settings.json (in your repo) to fully force-disable .{ts,tsx} parsing, but I'm hesitant to raise an issue there for now, since there's a limit to how many incompatibilities we can anticipate anyway.

echocrow commented 4 months ago

@kitten repro available here:

As per your comment, I suspect that this is a bug in the GraphQL LSP, validating gql.tada's GraphQL code, even when JS/TS files are not part of the configured documents.

Issue opened here:

Until (if?) this is addressed, I'd still recommend calling out this caveat in the gql.tada docs. The only workaround we've found so far was to outright disable the VSCode extension.

kitten commented 4 months ago

@echocrow: we don't have editor-specific docs yet and I have a feeling that's something we'd want to avoid for as long as possible for instead leaning into TS LSP+plugin support.

The problem is, as you can see in the WebStorm issue, it's often external problems and it's unclear how people arrive at solutions. We may even be talking about very personal setups here. While VSCode is the most common, there's also issues with the extension marketplace (for instance the official extensions aren't even the first that pop up in the search)

So, for now, the warnings have been added to the gql.tada doctor command instead, since there we can add setup-specific rules and checks

echocrow commented 4 months ago

we don't have editor-specific docs yet

except you kinda do 😅 (there is a VSCode specific note RE using the workspace version of TypeScript; but I understand the aversion, and arguably that note is more crucial and less niche.)

I was picturing a similar note (even if buried on the Fragment Colocation page) that other GraphQL LSPs should be configured to not check these ts(x) files, as they will incorrectly flag gql.tada's features as errors, and compete with gql.tada's auto-completion. (the latter is less of an issue, but we were also confused why ever field was suggested twice, thinking we spotted a minor gql.tada bug. now we know that both gql.tada and GraphQL LSP were echoing suggestions at the same time.) just thought this could be a useful note to other newcomers who may already be using an LSP in the same repo.

in any case, won't press the matter further! 🫡 thanks for the time and support! we have the LSP disabled for now, and hope the extraneous file checking gets fixed sometime (or we migrate fully to gql.tada before!)

(and sorry again @ OP for completely hijacking this issue 🙃)