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] Support JS/TS files as schema files #3587

Open
addiebarron opened this issue Apr 19, 2024 · 4 comments
Open

[lsp-server] Support JS/TS files as schema files #3587

addiebarron opened this issue Apr 19, 2024 · 4 comments
Labels
enhancement lsp-server graphql-language-service-server

Comments

@addiebarron
Copy link

addiebarron commented Apr 19, 2024

I left a related issue on the graphql-config repository: kamilkisiela/graphql-config#1425

Current Behavior (if applicable)

I work on a project that defines its GraphQL schema in gql tags across a number of .ts files in the server directory. We generate a schema.graphql file from those code files in the pre-commit step using graphql-codegen, but during development the most up-to-date information about the schema is in those gql tags, not in the schema.graphql file.

In VSCode, clicking through any client-side definitions (e.g. command-clicking a query name in an operation) sends me to the generated schema rather than the typescript (which is what we'd consider the "source of truth").

Desired Behavior

If JS/TS files are passed to the graphql-config schema field, VSCode should construct a schema from the gql tags in those files and use that as the source of truth.

This could be accomplished using the CodeFileLoader included in graphql-tools.

@addiebarron addiebarron added enhancement lsp-server graphql-language-service-server labels Apr 19, 2024
@acao
Copy link
Member

acao commented Apr 21, 2024

hey I hope to make a relase of the phase 1 rewrite today to solve this problem! it will be a new major release

@addiebarron
Copy link
Author

addiebarron commented Apr 21, 2024

@acao that's amazing!! could you clarify a bit more about how it will be solved? I noticed it's sort of possible on the pre-release branch, but it's pretty slow, and the click-through doesn't link to the correct line (it seems like it calculates the definition's line within the gql tag, then links to that line within the file, so it's off by the number of lines between the start of the file and the start of the gql tag).

@acao
Copy link
Member

acao commented Apr 21, 2024

@addiebarron oh dear, well that is not good news haha! i mean, it's great news in that it's the first unexpected bug report for the pre-release, as I have a few new test cases for what should be this scenario, so this is very very good to know something is missing or else I would have made a broken release!

Though that pre-release I uploaded is a bit out of date, the issue is likely still present. Can you provide a few example file snippets of the mix of config/schema/code files etc to reproduce the jump to definition offset bug?

Also, I'm sad to know it's much slower for you, I've been testing with a small group of maybe 12 files, so I was hoping to open a work project today as well, as I feared this could also be the case. A likely explanation for this (i will need to profile) is that many of the bugs I fix in this first stage of the rewrite involve properly updating and even writing to the cache when we were not before, due to bugs that just weren't caught in the unit test because the MessageProcessor instance lifecycle was too short (as unit tests should be!).

Thus why I added the new high-level integration suite at essentially the LSP protocol level, and thus revealed all these cache misses and issues that should not be happening (and tracking down the root and fixing, etc)! What's more, these caches and the parsing are in duplicate because of some flaws I inherited and haven't taken up to reconcile or worsened accidentally (until rewrite phase 2, where I consolidate the file and schema caches, and consolidate the parsing to )

As you may agree with your expertise, increased parsing and writes to and duplications of the various in memory caches means both way more in memory cacheing.

Also, a benchmark suite should be added to measure performance before this release. Some memory profiling with a larger project might help me track down some issues to target with the benchmarking. Initial project load (even with 12 files) and the definition lookups seemed slower to me, what else feels slower to you? Just everything? How many code files with/without graphql tags approximately? is there anything special about the schema, for example custom schema extensions?

(also btw this means i am not releasing! and it is soon monday here anyways 😆 ) I need to take more time with this, and if the second stage of the rewrite proves to be more performant measurably than the 1st stage, then perhaps we will join them together, perhaps as separate PR merges but the same major release. if both prove to be significantly worse off than what's currently stable, then first I timebox targeting the perf issues, and if they persist without major changes then I go back to the drawing board, because performance is already an issue, I can't bring myself to make it worse!

If you wanted to try the 2nd stage rewrite out with vscode, and/or tinker with it (PRs against my PRs or main are welcome ofc!), you could take these steps:

  1. clone
  2. yarn, make some coffee 🥱
  3. run the VSCode LSP Extension under "Run and Debug" dropdown in vscode
  4. select the project you want to test with in a new window
  5. when you want to test changes, just click the refresh button in the source editor

@acao
Copy link
Member

acao commented Apr 21, 2024

Also thank you so much for taking the time to try out the pre-release, you saved us all so much pain haha

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

No branches or pull requests

2 participants