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

Add full support for 'go to references' #3254

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Add full support for 'go to references' #3254

wants to merge 4 commits into from

Conversation

acao
Copy link
Member

@acao acao commented Jun 21, 2023

clone of #3250 with some desired changes and merging upstream

  • add more tests
  • add support for fragments
  • add support for searching both the current document and the schema for references
    • add support for searching all files for references
    • refactor graphql cache to cache documents instead of MessageProcessor? (big undertaking!) OR just move this logic to MessageProcessor as with getDefinitions
  • add tests for message processor

@changeset-bot
Copy link

changeset-bot bot commented Jun 21, 2023

🦋 Changeset detected

Latest commit: f7ccda0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
graphql-language-service Minor
graphql-language-service-server Minor
graphql-language-service-cli Minor
cm6-graphql Patch
codemirror-graphql Patch
@graphiql/react Patch
graphiql Patch
monaco-graphql Patch
vscode-graphql Patch
@graphiql/plugin-code-exporter Patch
@graphiql/plugin-explorer Patch
vscode-graphql-execution Patch
vscode-graphql-syntax Patch

Not sure what this means? Click here to learn what changesets are.

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

@acao acao force-pushed the find-references branch from 011e6ed to c8af187 Compare June 21, 2023 22:30
@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2023

The latest changes of this PR are available as canary in npm (based on the declared changesets):

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #3254 (f7ccda0) into main (5d06280) will decrease coverage by 0.08%.
The diff coverage is 50.90%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3254      +/-   ##
==========================================
- Coverage   56.39%   56.31%   -0.08%     
==========================================
  Files         109      109              
  Lines        5149     5190      +41     
  Branches     1405     1412       +7     
==========================================
+ Hits         2904     2923      +19     
- Misses       1822     1839      +17     
- Partials      423      428       +5     
Impacted Files Coverage Δ
...ql-language-service-server/src/MessageProcessor.ts 45.33% <29.16%> (-0.90%) ⬇️
...guage-service-server/src/GraphQLLanguageService.ts 45.57% <67.74%> (+3.42%) ⬆️

@acao acao force-pushed the find-references branch from c8af187 to 89ff700 Compare June 21, 2023 22:36
@acao acao force-pushed the find-references branch 2 times, most recently from 098be25 to b262595 Compare June 22, 2023 07:44
@acao acao force-pushed the find-references branch from b262595 to 1e429e6 Compare June 22, 2023 07:51
@acao acao changed the title Add basic support for 'go to references' Add full support for 'go to references' Jun 22, 2023
Copy link
Contributor

@AaronMoat AaronMoat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running the debug build, I get this weird GraphQL request / fake file in the references list in vscode. Do you get the same?

image

'graphql-language-service-cli': minor
---

Add basic support for 'go to references'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Add basic support for 'go to references'
Add support for 'go to references'

(given you'e expanded on it)

@acao
Copy link
Member Author

acao commented Jun 22, 2023

@AaronMoat yes, it's the schema file. I will come back later this evening to iterate over the project documents like we do for symbols/definitions. Currently, the project file cache lives in MessageProcessor, but we can use graphqlProject.getDocuments() as long as the documents are valid, but then we are reading the files directly instead of using the cache that updates only when the watched files are changed.

What I'm hoping to refactor is moving the document cache from MessageProcessor to GraphQLCache, but this will require refactoring much of the codebase and 4 different language features and more than 50% of the tests, but it is badly needed! This is a design flaw I inherited about 6 years ago and have allowed to continue to haunt the codebase haha

@acao acao marked this pull request as draft June 22, 2023 15:12
@acao
Copy link
Member Author

acao commented Jun 22, 2023

I'm putting together some diagrams and a writeup for this refactor to explain it better. Refactor is used broadly here, in the sense that the LSP outputs would be mostly the same, but the server needs a rewrite essentially. I may do this over the course of several PRs. I can use a branches from my fork if you'd like to contribute!

@AaronMoat
Copy link
Contributor

I'd be happy to help if you're looking for some, though I'll need guidance, still getting up to speed in this codebase :)

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

Successfully merging this pull request may close these issues.

2 participants