Simplify and consolidate LSP watcher registrations #1733

andrewbranch · 2025-09-19T21:41:32Z

In a large internal monorepo, this cut the number of individual globs we ask the LSP client to watch from 7667 (duplicated and narrower) globs to 7 (deduped and broader) globs, and cut the time it took to compute and register those globs by 80%. This is the basis for two follow-up fixes/improvements:

Change our registered globs to be extensionless in order to work around API: file watching doesn't report file deletions caused by directory deletion vscode#109754
Clear the file cache manually when a large number of files change in a short period of time, as some events are dropped by VS Code (LSP: changes to files not open in editor are sometimes not picked up #1646)

I want to get reviews on this first, as those fixes will build on this simplification.

andrewbranch · 2025-09-30T19:47:36Z

internal/project/projectcollectionbuilder.go

 				// !!! what about declaration files in node_modules? wouldn't it be better to
 				//     check project inclusion if the project is already loaded?
-				if !config.MatchesFileName(fileName) {
+				if _, ok := config.FileNamesByPath()[path]; !ok {


This was a drive-by fix, but MatchesFileName is a more expensive function that speculates about files that may not exist; the usage here was for a file that already exists and is either properly included by the config or not.

andrewbranch · 2025-09-30T19:49:37Z

internal/project/session_test.go

 			assert.Check(t, lsAfter.GetProgram() != programBefore)
 		})

+		t.Run("change program file not in tsconfig root files", func(t *testing.T) {


This test case exercises logic that was missing from the pre-snapshot LSP port.

andrewbranch · 2025-09-30T19:56:46Z

internal/project/watch.go

The removals in this file represent two strategy changes:

Since we rely on the LSP client to handle the watchers at the system level, remove our logic for avoiding installing watchers too close to the file system root. VS Code installs a watcher at the workspace level no matter that directory’s location, so we have nothing to gain by not using it. Any decision to refuse installing a watcher at a given location belongs to the client; they can simply return an error when we request to watch that location.

Prefer watching the workspace directory over anything else. If a file is in the workspace, don't register any additional watcher.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

andrewbranch · 2025-09-30T20:00:50Z

internal/tsoptions/parsedcommandline.go

 }

-// Porting reference: ProjectService.isMatchedByConfig
-func (p *ParsedCommandLine) MatchesFileName(fileName string) bool {


MatchesFileName was no longer being used except for the incorrect usage I pointed out above. When I initially implemented it for file watching, it was a bad performance bottleneck. PossiblyMatchesFileName is less precise but faster. It may be worth revisiting this if we invest into perf improvements of our include/exclude globbing (#1483)

jakebailey · 2025-09-30T20:54:37Z

internal/lsp/server.go

 	}

+	cwd := s.cwd
+	if s.initializeParams.Capabilities != nil &&


Boy do I want to add protobuf-style safe accessors for capabilities

jakebailey · 2025-09-30T22:27:59Z

I think this seems fine, but are we likely to want some of these heuristics back when we eventually do "real" watch mode for the CLI using fsnotify?

jakebailey · 2025-09-30T22:28:57Z

I guess the other concern I have is the part about watching all workspace files regardless of extension. I get that to VS Code, there's no difference, but we are still going to be sent all of those events...

I am sort of wondering if we should think about accelerating our trials of fsnotify to see if it works better than VS Code / the LSP client...

andrewbranch · 2025-09-30T22:36:23Z

I think it’ll be fine. We already handle batches of 30k+ changes during big npm installs fine, (and we’re going to end up ignoring huge batches like that and reloading everything in a future change anyway); I doubt opening things up to other file extensions will change things that much in projects that are already predominantly TS/JS.

sheetalkamat · 2025-10-01T18:10:23Z

internal/lsp/server.go

+		s.initializeParams.WorkspaceFolders != nil &&
+		s.initializeParams.WorkspaceFolders.WorkspaceFolders != nil &&
+		len(*s.initializeParams.WorkspaceFolders.WorkspaceFolders) == 1 {
+		cwd = lsproto.DocumentUri((*s.initializeParams.WorkspaceFolders.WorkspaceFolders)[0].Uri).FileName()


i think we should use all the workspace folders here?
#1238 where i had the change to handle this where we could infer project and its setting based on what path it is.

And also need to handle : lsproto.DidChangeWorkspaceFoldersParams:

it would also help with not looking past certain directory when finding tsconfig etc

Yeah, this is definitely incomplete but I want to handle multi-root workspace support all at once in a future PR. In theory, this shouldn’t break watching in other workspace folders, because we always check if the directories we need to watch are inside this directory, and fall back to other locations if not. But I’ve literally never tried tsgo in a multi-root workspace so I have no idea if anything works at all, at the moment.

what i mean is instead of using "cwd" as the deciding factor for watching strategy it would be nice to instead have something else handle that. I would rather now change cwd as part of this and keep that in play when nothing else is there? You can handle single workspace for now (and add multiple places to consider later) off of that option rather than changing cwd.

I do think this is kind of overloading the term "cwd" in an incorrect way, so I can rename/add a field to use instead. However, note that the actual process CWD was always / due to the way vscode-languageclient spawns the process. I thought about adding a CWD in the spawn options there, but I didn’t think there was an option that made sense, and we never use it for anything since every path that gets communicated over LSP is an absolute URI.

Yeah, the actual cwd of the process should never be used for any reason.

jakebailey · 2025-10-01T21:07:32Z

I went to try this out quick and was surprised by these watches:

[14:04:33.047] Added new watch: failed lookups for /home/jabaile/work/TypeScript/src/compiler/tsconfig.json watcher 12.1
[14:04:33.047] 	/home/**/*.{js,jsx,mjs,cjs,ts,tsx,mts,cts,json}
[14:04:33.047] 
[14:04:33.049] Added new watch: failed lookups for /home/jabaile/work/TypeScript/src/compiler/tsconfig.json watcher 12.2
[14:04:33.049] 	/node_modules/**/*.{js,jsx,mjs,cjs,ts,tsx,mts,cts,json}
[14:04:33.049]

That first one is saying to watch all files in all user dirs... As far as I know, editors will totally attempt to watch these and probably exhaust limits. Are we sure we don't want to cut off certain dirs or something?

andrewbranch · 2025-10-01T21:12:17Z

Hm, either there’s a bug in the logic that gets the common parents of the failed lookup locations, or lookups occurred in multiple users’ home directories. I should probably make the minimum depth stricter for directories that fall outside of the workspace directory, but do you think you could grab the list of failed lookups that generated that watch so I can see if the logic is right?

jakebailey · 2025-10-01T21:29:45Z

https://gist.github.com/jakebailey/84d5310acdb3397056009e0bbf998070 The big list of paths is me logging all of the files from createResolutionLookupGlobMapper (sorted).

andrewbranch added 7 commits September 18, 2025 13:47

Minimize number of watch globs

99939d9

Log watch update time

eebeee8

Update root files watch too

4acad53

Tweak workspace watching rules

31e58bf

Add failing test for watching file outside of tsconfig root files

4f1094d

Add non-root program file watch, fix watcher removal

20ab1fd

Lint

0761a15

andrewbranch mentioned this pull request Sep 22, 2025

LSP: changes to files not open in editor are sometimes not picked up #1646

Open

andrewbranch added 7 commits September 22, 2025 15:16

Fix GetCommonParents

a0af908

Use GetCommonParents instead of node_modules strategy

9369c83

Remove checks to see if locations are watchable

2c05e61

Fix bugs, make tests pass

be4e57f

Forgot to use PossiblyMatchesFileName and delete watcher parsed globs

fc75e42

Bad Copilot

da0795f

Fix test for updated signature

d8dab73

andrewbranch changed the title ~~Reduce number of LSP file watcher registrations~~ Simplify and consolidate LSP watcher registrations Sep 30, 2025

andrewbranch marked this pull request as ready for review September 30, 2025 19:45

Copilot AI review requested due to automatic review settings September 30, 2025 19:45

andrewbranch commented Sep 30, 2025

View reviewed changes

Revert formatting change

cb61cb8

andrewbranch commented Sep 30, 2025

View reviewed changes

Copilot AI reviewed Sep 30, 2025

View reviewed changes

andrewbranch commented Sep 30, 2025

View reviewed changes

Fix race

8755d8f

andrewbranch force-pushed the simplify-watch branch from eb8d310 to 8755d8f Compare September 30, 2025 20:10

Fix build

a32714e

andrewbranch requested review from jakebailey and sheetalkamat September 30, 2025 20:35

jakebailey reviewed Sep 30, 2025

View reviewed changes

sheetalkamat reviewed Oct 1, 2025

View reviewed changes

andrewbranch mentioned this pull request Oct 1, 2025

Port source map position mapping + go to definition source mapping #1767

Merged

Simplify and consolidate LSP watcher registrations #1733

Are you sure you want to change the base?

Simplify and consolidate LSP watcher registrations #1733

Uh oh!

Conversation

andrewbranch commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Sep 30, 2025

Uh oh!

jakebailey commented Sep 30, 2025

Uh oh!

andrewbranch commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewbranch Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Oct 1, 2025

Uh oh!

andrewbranch commented Oct 1, 2025

Uh oh!

jakebailey commented Oct 1, 2025

Uh oh!

Uh oh!

andrewbranch commented Sep 19, 2025 •

edited

Loading

andrewbranch Oct 1, 2025 •

edited

Loading