-
Notifications
You must be signed in to change notification settings - Fork 714
Simplify and consolidate LSP watcher registrations #1733
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
Changes from all commits
99939d9
eebeee8
4acad53
31e58bf
4f1094d
20ab1fd
0761a15
a0af908
9369c83
2c05e61
be4e57f
fc75e42
da0795f
d8dab73
cb61cb8
8755d8f
a32714e
af91353
95ea6b1
89586c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
"github.com/microsoft/typescript-go/internal/project" | ||
"github.com/microsoft/typescript-go/internal/project/ata" | ||
"github.com/microsoft/typescript-go/internal/project/logging" | ||
"github.com/microsoft/typescript-go/internal/tspath" | ||
"github.com/microsoft/typescript-go/internal/vfs" | ||
"golang.org/x/sync/errgroup" | ||
"golang.org/x/text/language" | ||
|
@@ -651,9 +652,27 @@ func (s *Server) handleInitialized(ctx context.Context, params *lsproto.Initiali | |
s.watchEnabled = true | ||
} | ||
|
||
cwd := s.cwd | ||
if s.initializeParams.Capabilities != nil && | ||
s.initializeParams.Capabilities.Workspace != nil && | ||
s.initializeParams.Capabilities.Workspace.WorkspaceFolders != nil && | ||
ptrIsTrue(s.initializeParams.Capabilities.Workspace.WorkspaceFolders) && | ||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we should use all the workspace folders here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And also need to handle : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would also help with not looking past certain directory when finding tsconfig etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the actual cwd of the process should never be used for any reason. |
||
} else if s.initializeParams.RootUri.DocumentUri != nil { | ||
cwd = s.initializeParams.RootUri.DocumentUri.FileName() | ||
} else if s.initializeParams.RootPath != nil && s.initializeParams.RootPath.String != nil { | ||
cwd = *s.initializeParams.RootPath.String | ||
} | ||
if !tspath.PathIsAbsolute(cwd) { | ||
cwd = s.cwd | ||
} | ||
|
||
s.session = project.NewSession(&project.SessionInit{ | ||
Options: &project.SessionOptions{ | ||
CurrentDirectory: s.cwd, | ||
CurrentDirectory: cwd, | ||
DefaultLibraryPath: s.defaultLibraryPath, | ||
TypingsLocation: s.typingsLocation, | ||
PositionEncoding: s.positionEncoding, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -343,14 +343,14 @@ func (b *projectCollectionBuilder) DidUpdateATAState(ataChanges map[tspath.Path] | |
// the set of typings files is actually different. | ||
p.installedTypingsInfo = ataChange.TypingsInfo | ||
p.typingsFiles = ataChange.TypingsFiles | ||
fileWatchGlobs, directoryWatchGlobs := getTypingsLocationsGlobs( | ||
typingsWatchGlobs := getTypingsLocationsGlobs( | ||
ataChange.TypingsFilesToWatch, | ||
b.sessionOptions.TypingsLocation, | ||
b.sessionOptions.CurrentDirectory, | ||
p.currentDirectory, | ||
b.fs.fs.UseCaseSensitiveFileNames(), | ||
) | ||
p.typingsFilesWatch = p.typingsFilesWatch.Clone(fileWatchGlobs) | ||
p.typingsDirectoryWatch = p.typingsDirectoryWatch.Clone(directoryWatchGlobs) | ||
p.typingsWatch = p.typingsWatch.Clone(typingsWatchGlobs) | ||
p.dirty = true | ||
p.dirtyFilePath = "" | ||
}, | ||
|
@@ -535,7 +535,7 @@ func (b *projectCollectionBuilder) findOrCreateDefaultConfiguredProjectWorker( | |
// For composite projects, we can get an early negative result. | ||
// !!! 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a drive-by fix, but |
||
node.logger.Log("Project does not contain file (by composite config inclusion)") | ||
return false, false | ||
} | ||
|
@@ -793,7 +793,8 @@ func (b *projectCollectionBuilder) updateProgram(entry dirty.Value[*Project], lo | |
if result.UpdateKind == ProgramUpdateKindNewFiles { | ||
filesChanged = true | ||
if b.sessionOptions.WatchEnabled { | ||
failedLookupsWatch, affectingLocationsWatch := project.CloneWatchers() | ||
programFilesWatch, failedLookupsWatch, affectingLocationsWatch := project.CloneWatchers(b.sessionOptions.CurrentDirectory, b.sessionOptions.DefaultLibraryPath) | ||
project.programFilesWatch = programFilesWatch | ||
project.failedLookupsWatch = failedLookupsWatch | ||
project.affectingLocationsWatch = affectingLocationsWatch | ||
} | ||
|
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.
Boy do I want to add protobuf-style safe accessors for capabilities