-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
(feat) watch tsconfig and extended tsconfig #1535
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.
This is great! About testing this: I don't know either how else to test this other than either with a try-catch-finally and some hacky overwriting of ts.sys
, or refactoring it so that we can test these things easier. I slightly prefer the "hack now, fix later" approach - in the long run it might be benefitial to make ts
a paramater of all functions/classes that uses, which would mean people could more easily use their workspace version of TS, and maybe it also makes some parts of the browser support easier (you know more about that one than me, just a guess).
|
||
extendedConfigToTsConfigPath.get(extendedConfig)?.forEach((config) => { | ||
services.delete(config); | ||
pendingReloads.add(config); |
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.
I was at first confused why this is enough, until I realized that you don't need to explicitly restart the server because the next getService
call will do this. Could you add a comment here and in watchConfigCallback
about this?
} | ||
|
||
return service; | ||
} | ||
|
||
function updateExtendedConfigDependents(service: LanguageServiceContainer) { |
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.
Code style: I would prefer it to have these added top level functions (at least the first two) inside createLanguageService
. This would remove the need to expose extendedConfigPaths
reusing watcher would cause memory leak
Is this ready for a final review and merge? It doesn't have tests now but I'm okay with this being untested for now |
I added some tests with the virtual |
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.
Nice!
I feel like we should revisit service.ts
at some point, it feels like it has grown a little too much over the years, but I don't have a good idea how to refactor this into something that is easier to understand and more testable.
#1529
Steps:
extendedConfigCache
to monitor referencedextendedConfig
todo:
find a way to test this. currently thinking about adding another property to
LanguageServiceDocumentContext
. To allow mocking thets.sys
object so we can more easily test it without needing totry finally
on the file system