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

Support queries for multiple schemas in the same file #3411

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

frandiox
Copy link

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:

image

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>

image image

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 with gql 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!

@changeset-bot
Copy link

changeset-bot bot commented Aug 24, 2023

⚠️ No Changeset found

Latest commit: 92f2c7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -288,6 +288,39 @@ export class MessageProcessor {
);
}

async _getDiagnosticsForAllFileProjects(
Copy link
Author

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');
Copy link
Author

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.

@frandiox frandiox changed the title feat: support queries for multiple schemas in the same file Support queries for multiple schemas in the same file Aug 24, 2023
if (config) {
return config;
getAllProjectsForFile(uri: Uri) {
const filePath = uri.startsWith('file:') ? fileURLToPath(uri) : uri;
Copy link
Author

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.

Copy link
Member

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

@acao
Copy link
Member

acao commented Sep 26, 2023

never thought to implement this because graphql-config didn't seem to support it, but here we are and it's a great idea.
as you may have noticed, isolated projects end up accidentally sharing schema as a bug in many contexts, so I'm glad you've added projects to the test mock as I'm planning to fix that as well.

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!

CC @dotansimha @Urigo

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Attention: Patch coverage is 84.61538% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 56.01%. Comparing base (29e99a8) to head (12acf3e).

❗ Current head 12acf3e differs from pull request most recent head 92f2c7b. Consider uploading reports for the commit 92f2c7b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
...raphql-language-service-server/src/GraphQLCache.ts 50.88% <100.00%> (+2.12%) ⬆️
...hql-language-service-server/src/findGraphQLTags.ts 82.71% <100.00%> (-1.90%) ⬇️
...guage-service-server/src/GraphQLLanguageService.ts 45.45% <82.35%> (+3.65%) ⬆️
...ql-language-service-server/src/MessageProcessor.ts 46.97% <84.84%> (+1.31%) ⬆️

... and 7 files with indirect coverage changes

@benjaminsehl
Copy link

Any chance you could take a look at this @acao? 🙏

@frandiox
Copy link
Author

frandiox commented Dec 11, 2023

I've merged main branch and resolved conflicts. Tests are still passing ✅

@acao
Copy link
Member

acao commented Dec 11, 2023

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.

@frandiox
Copy link
Author

Understandable! It would be great if you could mention this at the January WG 🙌
In the meantime, is there any public list of use cases that are normally tested manually? That would help us and the community to contribute with more unit/e2e test cases whenever possible.

@acao
Copy link
Member

acao commented Jan 7, 2024

@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!

@acao
Copy link
Member

acao commented Jan 7, 2024

oh and, can you add documention of this feature to the language server and vscode-graphql readmes?

@frandiox
Copy link
Author

frandiox commented Jan 8, 2024

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 languageService.gqlTagOptions.annotationSuffix public API? Or prefer to rename it / unnest it?

Copy link
Contributor

@TallTed TallTed left a 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/vscode-graphql/README.md Outdated Show resolved Hide resolved
packages/vscode-graphql/README.md Show resolved Hide resolved
packages/vscode-graphql/README.md Show resolved Hide resolved
@frandiox
Copy link
Author

frandiox commented Jan 11, 2024

A little human-facing language clarification. If it seems I've gotten any wrong, that's a good sign more clarification is needed. :-)

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);
Copy link
Member

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

Copy link
Author

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.

@frandiox
Copy link
Author

@acao Hi! I see there was a major refactoring in main branch, and that there's another PR on the way with more changes.

Do you think it makes sense to update this PR now and resolve conflicts or wait until new notice?

@TallTed
Copy link
Contributor

TallTed commented Jun 19, 2024

[@TallTed] A little human-facing language clarification. If it seems I've gotten any wrong, that's a good sign more clarification is needed. :-)

[@frandiox] 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

GitHub's change request interface includes an "add commit to batch" or similar button for you to use. If you don't use that button (i.e., you apply the change manually), it's difficult to be sure that change requests have been fully applied, and people like me have to review the current PR character-by-character.

As it stands, it appears that you've rejected all the change requests that are now "marked resolved". There's no argument in place, saying why some change request has been rejected, so there's no argument I can make for applying them, nor any way for me to revise my request.

If you do have good reason to reject any request, please state it inline, or use that change request to start a new issue, such that the work I've already done here is not wasted, and all interested parties can weigh in.

(As to whether my change requests are relevant to this PR -- I generally confine my suggestions to the lines changed by any given PR, though sometimes they extend to a nearby line, which is displayed in GitHub's "Files changed" pane. My change requests are generally human-focused and non-normative. There shouldn't be a problem with including such changes in any PR, even if totally unrelated. However, if you feel that any given change request does deserve to be in a separate PR, you can always use that change request as the start of a new issue, as described earlier.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants