Skip to content

Watch extended configs if present #41493

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 11, 2020
Merged

Watch extended configs if present #41493

merged 10 commits into from
Dec 11, 2020

Conversation

molisani
Copy link
Contributor

@molisani molisani commented Nov 11, 2020 •
edited
Loading

Fixes #17753

Inspects extendedSourceFiles from configFile in CompilerOptions and starts a watcher for each file. Watch callback invokes scheduleProgramReload just like the watcher for the main config file. Watcher pattern modelled after watcher map for missing files. Also provides shared watcher map for situations where multiple projects are open at once (tsbuild/tsserver).

Added new test cases, including chain of extended configs and ones copied from watchEnvironment/watchOptions with extended configs to verify they are being watched.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Nov 11, 2020
@molisani molisani force-pushed the watch-tsconfig-extends branch from bdf7c2e to ad2fbaa Compare November 11, 2020 17:59
sheetalkamat
sheetalkamat requested changes Nov 11, 2020
View reviewed changes
@molisani molisani force-pushed the watch-tsconfig-extends branch from 053e9b4 to 31f0d50 Compare November 13, 2020 03:26
molisani
molisani commented Nov 13, 2020
View reviewed changes
@molisani molisani force-pushed the watch-tsconfig-extends branch from 31f0d50 to 4d59cc6 Compare November 13, 2020 04:06
sheetalkamat
sheetalkamat requested changes Nov 13, 2020
View reviewed changes
@molisani molisani force-pushed the watch-tsconfig-extends branch from 588bee3 to 36c29cb Compare November 14, 2020 06:31
sheetalkamat
sheetalkamat requested changes Nov 17, 2020
View reviewed changes
@JoostK JoostK mentioned this pull request Nov 18, 2020
Closed
@molisani molisani force-pushed the watch-tsconfig-extends branch from d091cb9 to 4224dd7 Compare November 19, 2020 22:42
@molisani
Copy link
Contributor Author

molisani commented Nov 19, 2020 •
edited
Loading

I've rebased onto master, but I'm now getting some (seemingly) unrelated errors with some jsx baselines. Here are my local copies of the two failing baseline tests: jsFileCompilationTypeArgumentSyntaxOfCall.errors.txt and jsxCheckJsxNoTypeArgumentsAllowed.errors.txt. I'm going to do some more investigating, but wanted to report my local copies here on the PR if anyone could advise in this situation. This appears to be a known issue that is being fixed by #41602

@molisani molisani force-pushed the watch-tsconfig-extends branch 2 times, most recently from 67dcc8e to 86905be Compare November 20, 2020 03:38
sheetalkamat
sheetalkamat reviewed Dec 2, 2020
View reviewed changes
sheetalkamat
sheetalkamat requested changes Dec 2, 2020
View reviewed changes
src/compiler/watchUtilities.ts Outdated
) {
const extendedSourceFiles = configFile.extendedSourceFiles || emptyArray;
// TODO(rbuckton): Should be a `Set` but that requires changing the below code that uses `mutateMap`
const newExtendedConfigFilesMap = arrayToMap(extendedSourceFiles, identity, returnTrue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key needs to be Path, value needs to be file name to watch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnot create the key with Path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed the meaning of this the first time around. I'll pass something to allow converting from string to Path.

molisani
molisani commented Dec 2, 2020
View reviewed changes
Copy link
Contributor Author

@molisani molisani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update to correctly account for removals, and add new tests. Had a few questions about latest review.

@molisani
Copy link
Contributor Author

molisani commented Dec 4, 2020

While creating a test case for removing projects, I noticed that I had not considered the builder's watch logic. I'll replicate the "shared" watcher pattern from the server here as well.

@molisani molisani force-pushed the watch-tsconfig-extends branch 2 times, most recently from 1acd39c to 2284a6c Compare December 9, 2020 22:49
@molisani molisani requested a review from sheetalkamat December 9, 2020 23:02
sheetalkamat
sheetalkamat requested changes Dec 10, 2020
View reviewed changes
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some more feedback but i am going to try fixing some of those myself as that might be easier

src/compiler/watchUtilities.ts Outdated
) {
const extendedSourceFiles = configFile.extendedSourceFiles || emptyArray;
// TODO(rbuckton): Should be a `Set` but that requires changing the below code that uses `mutateMap`
const newExtendedConfigFilesMap = arrayToMap(extendedSourceFiles, identity, returnTrue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnot create the key with Path

@molisani
Copy link
Contributor Author

I have some more feedback but i am going to try fixing some of those myself as that might be easier

Thank you very much for those suggestions. I merged all of them and then applied the toPath conversion.

@sheetalkamat
Copy link
Member

Please cherry pick e2d178f as i dont have permission to update your fork.

@molisani
Watch extended configs if present
c0b6fb5
@molisani
Address code review comments
55d4e3e 
Added new `WatchType` for extended config files. Refactored watch map
update to separate function, relocated call sites. Removed unnecessary
test cases and relocated with new tests in programUpdates.
molisani and others added 8 commits December 10, 2020 19:46
@molisani
Unify extended config file watching between tsc/tsserver
a6a3bc2 
Update `updateExtendedConfigFilesWatch` to read from a
`TsConfigSourceFile` to get `extendedSourceFiles`. Add watcher map to
`ConfiguredProject` in the server. New test cases to verify correct
events triggered and extended files are being watched properly.
@molisani
Simplify watcher callback, fix tests
fa918ed 
Removes unnecessary actions in extended config watcher callback
function. Updates tests to match.
@molisani
Share extended config watchers across projects in server
1a527d6 
New shared watcher map in ProjectService that stores callbacks per
project to be invoked when the file watcher is triggered. The
FileWatcher is created with the watch options of the first Project to
watch the extended config.
@molisani
Refactor shared extended config map and watchers
963ccf9 
Remove all server-related utility functions/types from
watchUtilities. Store config-project mapping and config file watchers
inside ProjectService with new private methods to add or remove
projects.
@molisani
Store projects in extended config file watcher
c34125b 
Creates SharedExtendedConfigFileWatcher in both editorServices
(tsserver) and tsbuildPublic. The file watcher is responsible for
triggering a full project reload for the contained projects. Upon
reload, any configs that are no longer related to a project have their
watchers updated to match. New test cases to confirm that the file
watchers for extended configs are closed when the project is closed.
@molisani @sheetalkamat
Apply suggestions from code review
e7ff4b8 
Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>
@molisani
Map extended config files by path
45f19cc
@sheetalkamat @molisani
Move shared watcher into utilities and add more tests
7d25e43
@molisani molisani force-pushed the watch-tsconfig-extends branch from 3a0c0dd to 7d25e43 Compare December 11, 2020 00:49
@molisani
Copy link
Contributor Author

Please cherry pick e2d178f as i dont have permission to update your fork.

The unified SharedExtendedConfigFileWatcher makes a lot more sense now that it's being used by tsbuild and tsserver. I also had to rebase due to a merge conflict in src/server/project.ts but it was a small one. Thanks again for your help.

@sheetalkamat sheetalkamat merged commit 716b167 into microsoft:master Dec 11, 2020
@molisani molisani deleted the watch-tsconfig-extends branch December 11, 2020 15:31
@MartinJohns MartinJohns mentioned this pull request Oct 30, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sheetalkamat sheetalkamat sheetalkamat approved these changes

Assignees

@sheetalkamat sheetalkamat

Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Apart from watching config file for the project updates, we probably should also watch extended config file
3 participants
@molisani @sheetalkamat @typescript-bot