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
Support queries for multiple schemas in the same file #3411
base: main
Are you sure you want to change the base?
Conversation
|
@@ -288,6 +288,39 @@ export class MessageProcessor { | |||
); | |||
} | |||
|
|||
async _getDiagnosticsForAllFileProjects( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is extracted from the code below to avoid repetition, and it's adapted to run for every project that matches the file.
const hasGraphQLPrefix = | ||
node.quasis[0].value.raw.startsWith('#graphql\n'); | ||
const hasGraphQLPrefix = node.quasis[0].value.raw.startsWith('#graphql'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the matching similar to the syntax highlight here, allowing #graphql:suffix
.
if (config) { | ||
return config; | ||
getAllProjectsForFile(uri: Uri) { | ||
const filePath = uri.startsWith('file:') ? fileURLToPath(uri) : uri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing an Uri that starts with the file://
protocol doesn't work properly in grahiQLConfig.getProjectForFile
. Internally that calls minimatch
and it always returns false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there was a breaking change in vscode some years ago, and there are a few places that we didn't handle this, thanks for the bugfix! I will come back around look at the implementations because they should all be passing a file url
never thought to implement this because graphql-config didn't seem to support it, but here we are and it's a great idea. I will focus on a few bugfixes this week, but with my current day job workload, I can't promise I'll have time to fully review this until next week. The lack of test coverage makes things precarious and there are a number of manual scenarios I like to check, but it seems you've already fixed a few known bugs yourself! And added coverage, thanks for that! |
b20c7ed
to
bbc8a6e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3411 +/- ##
==========================================
+ Coverage 55.33% 56.01% +0.67%
==========================================
Files 115 114 -1
Lines 5349 5311 -38
Branches 1450 1441 -9
==========================================
+ Hits 2960 2975 +15
+ Misses 1963 1909 -54
- Partials 426 427 +1
|
packages/graphql-language-service-server/src/GraphQLLanguageService.ts
Outdated
Show resolved
Hide resolved
Any chance you could take a look at this @acao? 🙏 |
I've merged main branch and resolved conflicts. Tests are still passing ✅ |
unfortunately this impacts a lot of behavior for which there is no test coverage, so an exhaustive set of manual tests will be necessary. If it makes sense to introduce this after review with the spec committees, I should be able to get to it in the coming months. I hope to present it at the January WG, or perhaps you can present it at the next graphql working group? There is some high priority stability & test work that needs to come before major changes, as requested by the TSC and other stakeholders. Though the LSP is only at about 50% coverage, even 100% coverage won't account for all the scenarios we need to cover before we can make major changes. |
Understandable! It would be great if you could mention this at the January WG 🙌 |
@frandiox the readme for the language server and the graphql config website document many combinations of test cases we don't test for also if you can update the PR, i refactored tag parsing and improved types in the language server some. I think we can get this improvement out there sooner than later! |
oh and, can you add documention of this feature to the language server and vscode-graphql readmes? |
Done! Let me know if you prefer docs in some other way. Also, are we OK with the suggested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little human-facing language clarification. If it seems I've gotten any wrong, that's a good sign more clarification is needed. :-)
packages/graphql-language-service-server/src/findGraphQLTags.ts
Outdated
Show resolved
Hide resolved
Hi 👋 Thanks for the suggestions. It looks like most of the changes are not related to this PR, though. Should we move it to a different PR? Not sure how strict things should be with PRs in this repo cc @acao |
@@ -111,7 +149,7 @@ export class GraphQLLanguageService { | |||
// Perform syntax diagnostics first, as this doesn't require | |||
// schema/fragment definitions, even the project configuration. | |||
let documentHasExtensions = false; | |||
const projectConfig = this.getConfigForURI(uri); | |||
const projectConfig = this.getProjectForQuery(document, uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename this to getProjectForDocument
? just so people don't assume this only works with queries, as it should also work with SDL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! I've also resolved the new conflicts but fyi, there's a problem with cspell
on commit because it finds unknown words in .vscode/settings.json
.
Hi 👋 First of all, thanks so much for the active maintenance of these packages!
While working on shopify/hydrogen I noticed that the GraphQL Language Service Server so far only supports 1 schema per file. However, we are introducing multiple GraphQL APIs and it would be nicer for DX to combine queries in the same file instead of separating them in different files.
This is an attempt to implement that. I was just trying to see if it was possible at all to open an issue, but it actually works so I'm just opening a PR instead 😅 -- feel free to discuss this feature completely!
The idea is that you need a config like the following with 2 projects pointing to the same files. Then, you can pass an option to set a "gql annotation suffix" in
extensions.languageService.gqlTagOptions.annotationSuffix
:Then, in a given file that matches the pattern for both projects, you can choose to which GraphQL API/schema each of the queries points to with a modifier of the GraphQL annotation:
#graphql:<suffix>
This whole thing also works with GraphQL Codegen by using
documentTransforms
. Also, noticed that syntax highlights already works due to how the match happens in that package (doesn't end in\n
).This feature at the moment only works when using the in-query comment
#graphql
. It doesn't work withgql
tag or the leading comment/* GraphQL */
because there's no access to that information where it's needed (maybe that could be changed? Unsure).Another option is to just read a different comment inside the query, like
#project:storefront
and make it independent from the#graphql
comment.Thanks for checking it!