- Notifications
You must be signed in to change notification settings - Fork664
Watch type refs and instead of watching directly use go routine#1217
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors module resolution watching by extracting lookup-glob generation into standalone functions, introduces concurrency-safe watcher updates withsync.RWMutex, and runs watcher updates and typing-location watches in goroutines.
- Introduced
getModuleResolutionWatchGlobsand genericextractLookupsinwatch.go - Added
watchMu sync.RWMutexto guard watcher updates inproject.goand madeupdateWatchersasynchronous - Spawned goroutines for
updateWatchersandWatchTypingLocationsto decouple from the main update loop
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/project/watch.go | New helper functions to gather module-resolution watch patterns |
| internal/project/project.go | AddedwatchMu, updatedupdateWatchers signature and concurrency, adjusted locks inWatchTypingLocations and event handler |
| internal/project/ata.go | ChangeddiscoverAndInstallTypings to invokeWatchTypingLocations asynchronously |
| internal/module/types.go | AddedGetLookupLocations methods to resolved-module types |
| internal/compiler/program.go | ExposedGetResolvedTypeReferenceDirectives onProgram |
| internal/compiler/parsetask.go | Added flag for automatic-type parsing inparseTask |
| internal/compiler/fileloader.go | Integrated automatic-type-directive tasks into parsing workflow |
Comments suppressed due to low confidence (3)
internal/project/project.go:354
- [nitpick] The parameter name
programshadowsp.program; consider renaming it (e.g.,prog) to reduce confusion.
func (p *Project) updateWatchers(ctx context.Context, program *compiler.Program) {internal/project/watch.go:368
- These new functions (
getModuleResolutionWatchGlobsandextractLookups) introduce nontrivial logic; consider adding unit tests to verify correct extraction of lookup globs.
func getModuleResolutionWatchGlobs(program *compiler.Program) (failedLookups map[tspath.Path]string, affectingLocaions map[tspath.Path]string) {internal/project/project.go:170
- The code introduces
sync.RWMutexbut there is nosyncimport; addimport "sync"to avoid a compile error.
watchMu sync.RWMutexUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
The file loader code seems correct to me, though I am unsure about the concurrency stuff in the Project given our ongoing logical races.
I assume no tests change with the loader change since the main effect here is to actually keep track of these files for watching?
No description provided.