feat: support vscode multi-root workspaces #2594

Foo-x · 2022-07-27T14:33:51Z

I reffered to the following official implementation.
https://github.com/microsoft/vscode-extension-samples/blob/main/lsp-multi-server-sample/client/src/extension.ts

graphql-language-service-server will be started for each folders, and the corresponding schema is used for each.

The schema for packages/app folder has foo.

The schema for packages/api folder does not have foo.

StatusBarItem and OutputChannel will be shared by all folders and updated according to the active editor.

changeset-bot · 2022-07-27T14:33:53Z

🦋 Changeset detected

Latest commit: ad87012

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

This PR includes changesets to release 1 package

Name	Type
vscode-graphql	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

linux-foundation-easycla · 2022-07-27T14:33:55Z

The committers listed above are authorized under a signed CLA.

✅ login: Foo-x (abbfad9)

acao · 2022-07-27T14:53:28Z

Wow ok, i hadn’t thought of starting multiple instances. I was just going to re-architect the server to handle each workspace in the same server. Does this work when multiple graphql config projects are defined in a file?

either way, awesome work and thank you for this!

codecov · 2022-07-27T14:56:08Z

Codecov Report

Merging #2594 (ad87012) into main (2d91916) will increase coverage by 3.90%.
The diff coverage is 23.84%.

@@            Coverage Diff             @@
##             main    #2594      +/-   ##
==========================================
+ Coverage   65.70%   69.61%   +3.90%     
==========================================
  Files          85       72      -13     
  Lines        5106     4275     -831     
  Branches     1631     1436     -195     
==========================================
- Hits         3355     2976     -379     
+ Misses       1747     1294     -453     
- Partials        4        5       +1

Impacted Files	Coverage Δ
packages/codemirror-graphql/src/lint.ts	`100.00% <ø> (ø)`
packages/codemirror-graphql/src/results/mode.ts	`47.05% <ø> (ø)`
packages/codemirror-graphql/src/utils/hintList.ts	`95.65% <ø> (ø)`
...ckages/codemirror-graphql/src/utils/mode-indent.ts	`0.00% <0.00%> (ø)`
packages/codemirror-graphql/src/variables/mode.ts	`79.48% <ø> (ø)`
packages/graphiql-react/src/editor/whitespace.ts	`100.00% <ø> (ø)`
packages/graphiql-react/src/utility/debounce.ts	`0.00% <0.00%> (ø)`
packages/graphiql-react/src/editor/tabs.ts	`5.66% <5.66%> (ø)`
packages/codemirror-graphql/src/variables/lint.ts	`47.61% <66.66%> (+0.63%)`	⬆️
packages/codemirror-graphql/src/hint.ts	`94.73% <100.00%> (ø)`
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5feda2...ad87012. Read the comment docs.

Foo-x · 2022-07-27T15:13:58Z

Sorry, I have not checked on multiple projects.
It is midnight here in Japan now, so please let me check tomorrow.

acao · 2022-07-27T15:49:49Z

@Foo-x hey that would be great, if I don’t get to it first! No hurry at all, but don’t be surprised if this gets released before you wake up!

thank you so much for this!

packages/vscode-graphql/src/extension.ts

acao · 2022-07-27T20:06:05Z

@Foo-x i think this is good to go, can you add a changeset file with your changelog entry as well?

it seems there are unrelated issues with graphiql-config multi-project anyways, introduced by me last year, and this should work in tandem with that programatically, my concern is just resource usage, but I will make the optimization to address that concern shortly

add a changeset for the changelog, so that the version increments

Foo-x · 2022-07-28T10:58:52Z

@acao Thank you for adding the changeset!

Regarding the behavior in multi-projects, I found a problem that intellisense is displayed twice.
Confirmed with following.

.
└── parent
    ├── .graphqlrc.json
    ├── alpha
    │   ├── alpha.graphql
    │   └── foo.tsx
    ├── beta
    │   ├── beta.graphql
    │   └── foo.tsx
    └── shared
        └── shared.graphql

parent/.graphqlrc.json

{
  "projects": {
    "alpha": {
      "schema": ["alpha/**", "shared/**"]
    },
    "beta": {
      "schema": ["beta/**", "shared/**"]
    }
  }
}

# shared.graphql
type Shared {
  shared: Boolean
}

# alpha.graphql
type Query {
  sharedAlpha: Shared
}

# beta.graphql
type Query {
  sharedBeta: Shared
  betaAnother: BetaType
}

type BetaType {
  foo: Boolean
}

on alpha/foo.tsx

on beta/foo.tsx

Let me see if I can fix it.

Foo-x · 2022-07-28T13:19:56Z

Fixed the problem above.

But there is yet another problem that if there is a GraphQL config file outside the nested folders, the inner folders always belong to the outside folder as a project.

.
└── parent
    ├── .graphqlrc.json
    ├── alpha
    │   ├── alpha.graphql
    │   └── foo.tsx
    ├── beta
    │   ├── beta.graphql
    │   └── foo.tsx
    ├── gamma
    │   ├── .graphqlrc.json
    │   ├── foo.tsx
    │   └── gamma.graphql
    └── shared
        └── shared.graphql

parent/.graphqlrc.json

{
  "projects": {
    "alpha": {
      "schema": ["alpha/**", "shared/**"]
    },
    "beta": {
      "schema": ["beta/**", "shared/**"]
    }
  }
}

on gamma/foo.tsx

If you don't add parent to your workspace, there is no problem.

I think the server needs to be implemented to solve this problem.

acao · 2022-07-28T13:40:10Z

Yes @Foo-x this is one of the longstanding known bugs in the LSP server I was referring to. You are free to try and fix it in the server in this PR, or we can fix it later. What you’ve implemented works great!

Foo-x · 2022-07-28T14:15:53Z

Ok, I'll try it!

acao · 2022-07-30T06:55:18Z

@Foo-x this is amazing work! Would you like to pair? I have a few more relatively simple optimisations we can add. Are you on the discord?

https://discord.com/channels/625400653321076807/863688312756371476

my mission this weekend is to get this and some other features and optimisations out to celebrate a million installs for vscode-graphql! your timing couldn't have been more perfect

acao · 2022-07-30T07:10:21Z

I would also love to explain some problematic architecture and hacks of mine that you're inevitably stumbling into 😆 , and how I envision this server could be architected better. For example:

I put way too much logic in MessageProcessor that should be in GraphQLCache or GraphQLLanguageService. Many lsp packages that use vscode-languageserver use a TextDocument class, though really we could just enhance GraphQLCache and update/invalidate entries on the proper workspace events, and remove the parsing of huge globs of files by GraphQLCache, and problem solved!

As you probably know this is the LSP client file-watcher pattern that we never fully moved to from the pre-LSP days of this language server. I adopted it after it had transitioned to LSP model, but at the time watchman was still used by the server. I removed that to adopt the client file watcher pattern, but I didn't set it up for all the correct events. For example, many users use codegen tools for graphql, so the cache entries need to happen on all workspace events obviously!

acao · 2022-07-30T07:15:25Z

@Foo-x also, another huge and relatively low-hanging optimization that leads to deleting a bunch of code is rewriting _updateGraphQLConfig() to only accept a specific project from the file. Then the logic needs to move from a single init to tracking which projects have "pre-initialized" or not.

This beast i see you've commented out:

 /**
   * This should only be run on initialize() really.
   * Caching all the document files upfront could be expensive.
   * @param config {GraphQLConfig}
   */
  async _cacheAllProjectFiles(config: GraphQLConfig) {
    if (config?.projects) {
      return Promise.all(
        Object.keys(config.projects).map(async projectName => {
          const project = config.getProject(projectName);
          await this._cacheSchemaFilesForProject(project);
          await this._cacheDocumentFilesforProject(project);
        }),
      );
    }
  }

good! this method should not exist! why did i do this 😆 ! you only need to warm the GraphQLCache for the project if it hasn't been already.

so, this cache/config/langauge service init step could be called once every time you open a file in a project that has not initialized before. This way, if you only open a file in one of your graphql config projects, only the cache for that project is built, and if enabled, only the SDL definition lookup for that project is cached in your fs

acao · 2022-07-30T07:23:30Z

I think it's possible if we do this, or something you feel is even more optimal, #2459 we can hopefully close as well, which is terrible, vscode-crashing perf issues on multi-project caused by this method!

Foo-x · 2022-07-30T08:00:24Z

I have never used discord, but I just installed it and joined the channel!
My account is foo-x#1064.
I can use it if needed.

And thank you for your very thorough explanation!
I understand a little better what needs to be fixed.
But I'm new to this repo, so I don't understand the architecture yet.
I'll take a look at it.
Then, I'll try to cache only specific project from the file.

acao · 2022-07-30T08:12:46Z

@Foo-x if you want to add that optimisation, then sure! I will reach out on discord

acao · 2022-07-30T09:32:58Z

packages/graphql-language-service-server/src/MessageProcessor.ts

@@ -89,58 +89,48 @@ function toPosition(position: VscodePosition): IPosition {
  return new Position(position.line, position.character);
 }

-export class MessageProcessor {
+class WorkspaceMessageProcessor {


Brilliant! An elegant solution

Would you be interested in separating the WorkspaceMessageProcessor into a separate file? the less cognitive overhead, the more useful it is as a reference implementation, the more accessible it is to contributors!

Thank you for your review!
Separated to new file.

acao

Everything here looks good to go! Thank you @Foo-x for helping with such a comprehensive PR!
Looking forward to more collaboration when you're able! 🚀

acao · 2022-07-31T17:15:16Z

@Foo-x ok I hate to do this, it's my fault really. I merged too quickly.

What I would like to do is see if you could re-push your branch if it's still on your local so we can revert this PR. Then I will merge #2573 and release, then merge a re-creation of this PR for the pre-release

This reverts commit 5827977.

Foo-x · 2022-08-01T00:28:45Z

@acao
Of course, you can revert my PR!
I have created new PR that revert this.
#2612

But how .changeset/thick-years-sort.md reverted since it has been changed on latter commits?
I simply deleted it on the PR above.
If there is another way, please let me know.

temporarily reverting this to release a stable vscode-graphql release first

…2612)" This reverts commit 621253d.

Introduces multi-root workspaces support using a single language server instance! Adds a few optimisations, tests, etc as well. This had previously been merged in a way that caused issues, but this should resolve those issues. Revert "Revert "feat: support vscode multi-root workspaces (#2594)" (#2612)" (#2616)

feat: support vscode multi-root workspaces

abbfad9

acao requested review from orta and stonexer July 27, 2022 14:54

acao reviewed Jul 27, 2022

View reviewed changes

packages/vscode-graphql/src/extension.ts Outdated Show resolved Hide resolved

add changeset

f8e1939

add a changeset for the changelog, so that the version increments

fix: language server duplication

64aff6d

feat: use single server

fd4952c

acao mentioned this pull request Jul 30, 2022

High CPU usage with GraphQL.vscode-graphql extension #2459

Open

acao marked this pull request as draft July 30, 2022 07:24

acao reviewed Jul 30, 2022

View reviewed changes

Foo-x added 2 commits July 30, 2022 19:40

refactor: separate WorkspaceMessageProcessor

757c91e

fix: tests

ad87012

Foo-x marked this pull request as ready for review July 30, 2022 11:44

acao self-requested a review July 30, 2022 13:31

acao approved these changes Jul 30, 2022

View reviewed changes

acao merged commit 5827977 into graphql:main Jul 30, 2022

acao mentioned this pull request Jul 30, 2022

Version Packages #2603

Merged

Foo-x deleted the vscode-multi-root branch July 30, 2022 13:41

Foo-x added a commit to Foo-x/graphiql that referenced this pull request Aug 1, 2022

Revert "feat: support vscode multi-root workspaces (graphql#2594)"

f527e41

This reverts commit 5827977.

acao pushed a commit that referenced this pull request Aug 1, 2022

Revert "feat: support vscode multi-root workspaces (#2594)" (#2612)

621253d

temporarily reverting this to release a stable vscode-graphql release first

acao added a commit that referenced this pull request Aug 1, 2022

Revert "Revert "feat: support vscode multi-root workspaces (#2594)" (#…

73d6f68

…2612)" This reverts commit 621253d.

acao added a commit that referenced this pull request Oct 8, 2022

Revert "Revert "feat: support vscode multi-root workspaces (#2594)" (#…

9306f66

…2612)" This reverts commit 621253d.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support vscode multi-root workspaces #2594

feat: support vscode multi-root workspaces #2594

Foo-x commented Jul 27, 2022

changeset-bot bot commented Jul 27, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 27, 2022 •

edited

Loading

acao commented Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 27, 2022 •

edited

Loading

Foo-x commented Jul 27, 2022

acao commented Jul 27, 2022 •

edited

Loading

acao commented Jul 27, 2022

Foo-x commented Jul 28, 2022

Foo-x commented Jul 28, 2022

acao commented Jul 28, 2022 •

edited

Loading

Foo-x commented Jul 28, 2022

acao commented Jul 30, 2022 •

edited

Loading

acao commented Jul 30, 2022 •

edited

Loading

acao commented Jul 30, 2022 •

edited

Loading

acao commented Jul 30, 2022

Foo-x commented Jul 30, 2022

acao commented Jul 30, 2022

acao Jul 30, 2022

acao Jul 30, 2022

Foo-x Jul 30, 2022

acao left a comment •

edited

Loading

acao commented Jul 31, 2022

Foo-x commented Aug 1, 2022

feat: support vscode multi-root workspaces #2594

feat: support vscode multi-root workspaces #2594

Conversation

Foo-x commented Jul 27, 2022

changeset-bot bot commented Jul 27, 2022 • edited Loading

🦋 Changeset detected

linux-foundation-easycla bot commented Jul 27, 2022 • edited Loading

acao commented Jul 27, 2022 • edited Loading

codecov bot commented Jul 27, 2022 • edited Loading

Codecov Report

Foo-x commented Jul 27, 2022

acao commented Jul 27, 2022 • edited Loading

acao commented Jul 27, 2022

Foo-x commented Jul 28, 2022

Foo-x commented Jul 28, 2022

acao commented Jul 28, 2022 • edited Loading

Foo-x commented Jul 28, 2022

acao commented Jul 30, 2022 • edited Loading

acao commented Jul 30, 2022 • edited Loading

acao commented Jul 30, 2022 • edited Loading

acao commented Jul 30, 2022

Foo-x commented Jul 30, 2022

acao commented Jul 30, 2022

acao Jul 30, 2022

Choose a reason for hiding this comment

acao Jul 30, 2022

Choose a reason for hiding this comment

Foo-x Jul 30, 2022

Choose a reason for hiding this comment

acao left a comment • edited Loading

Choose a reason for hiding this comment

acao commented Jul 31, 2022

Foo-x commented Aug 1, 2022

changeset-bot bot commented Jul 27, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 27, 2022 •

edited

Loading

acao commented Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 27, 2022 •

edited

Loading

acao commented Jul 27, 2022 •

edited

Loading

acao commented Jul 28, 2022 •

edited

Loading

acao commented Jul 30, 2022 •

edited

Loading

acao commented Jul 30, 2022 •

edited

Loading

acao commented Jul 30, 2022 •

edited

Loading

acao left a comment •

edited

Loading