[lsp-server] 🐞 JS/TS files should only be checked when included in `documents` config glob

echocrow commented 7 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current Behavior

It seems that the LSP checks and validates GraphQL code embedded in JS/TS files, even when .graphqlrc only lists *.graphql globs in documents. This even happens when documents explicitly excludes JS/TS files, e.g. via !**.{js,ts}.

Expected Behavior

No checks/validations should be run on GraphQL code inside of JS/TS files when a config with documents glob(s) is present, which does not include those JS/TS files (or even excludes them).

Steps To Reproduce

Repro: https://github.com/echocrow/gql-tada-vs-lsp-repro

Set up .graphqlrc config:

{
"schema": "src/my-schema.graphql",
"documents": "src/**/*.graphql"
}

Create index.ts:

const MyExample = graphql(`
fragment MyExample on Foobar @my_tool_directive {
foobar
}
`);

In this minimal example, the LSP will unexpectedly check the GraphQL code in this TS file and flag the following:

[GraphQL: Validation] Unknown directive "@my_tool_directive".

Environment

IDE: vscode v1.88.1
IDE Extension: GraphQL.vscode-graphql v0.9.3

Anything else?

Motivation:

There days, various tools allow us to embed GraphQL directly in JS/TS files. Sometimes, these tools also introduce new directives, or new ways of structuring fragments/queries/mutations.

However, this can leave to false-positive errors flagged by the GraphQL LSP when it validates JS/TS-embedded GraphQL code. It makes sense that it does not know about those tool-specific directives, but it is unexpected that the embedded GraphQL code is checked by the LSP at all.

acao commented 7 months ago

re: this, this problem has been fixed in a PR last month, the stage 2 rewrite of the LSP server, after releasing the first rewrite today, I will then prepare this other branch to consolidate all of this in stage 2 rewrite. TLDR i have spent about 400 hours preparing several rewrites to address this and many more issues!

you just need to make the directives available via introspection!

and if not you can add them to the graphql config

echocrow commented 7 months ago

cheers for the context, and all the hard work! sounds a fix for this is already in the pipeline. 🤞

if it's of any help, when available I'm happy to test the LSP rewrite for the issue reported here.

you just need to make the directives available via introspection!

totally agree if it was just directives! unfortunately the unexpected GraphQL LSP checking goes beyond directives thogh. piggybacking the repro from the OP, which demonstrates other tooling features:

When using e.g. gql.data, we can leverage separate Fragment definitions in JS. Those could be co-located, or could be shared and imported from other JS files. However, GraphQL LSP complains about the unknown fragment when used in another query. Given what the LSP knows, that makes sense; I don't know think it would be in GraphQL LSP's scope to understand and validate this rather tooling-specific setup.
The tool in this repo comes with its own autocomplete suggestions in the form of a TS plugin. With the GraphQL LSP also running in the same JS/TS file, however, they two unexpectedly compete. This results in a flood of duplicate query/mutation/field suggestions. Not critical, but a minor annoyance, and potentially a bit confusing at first.

graphql / graphiql