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

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

Open
1 task done
echocrow opened this issue Apr 20, 2024 · 2 comments
Open
1 task done
Labels
bug lsp-server graphql-language-service-server

Comments

@echocrow
Copy link

echocrow commented Apr 20, 2024

Is there an existing issue for this?

  • 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

  1. Set up .graphqlrc config:
{
  "schema": "src/my-schema.graphql",
  "documents": "src/**/*.graphql"
}
  1. 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

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.

@echocrow echocrow added bug lsp-server graphql-language-service-server labels Apr 20, 2024
@echocrow echocrow changed the title [lsp-server] 🐞 JS/TS files only be checked when included in documents config glob [lsp-server] 🐞 JS/TS files should only be checked when included in documents config glob Apr 20, 2024
@acao
Copy link
Member

acao commented Apr 21, 2024

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
Copy link
Author

echocrow commented Apr 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug lsp-server graphql-language-service-server
Projects
None yet
Development

No branches or pull requests

2 participants