Skip to content
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

Add way to exclude files and directories to watch #39243

Merged
merged 23 commits into from
Nov 4, 2020
Merged

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Jun 24, 2020

This allows user a way to specify directories and/or files to ignore:

  • 869abb1 Adds parsing of new watchOptions: excludeFiles and excludeDirectories in tsconfig as well as command line
  • 9cd3e93 Adds a way to create noopWatcher for files and directories that are being watched.
  • dddd906 Adds a way to exclude invoking callback for excluded things from sys itself. This means no invoke for excluded path if its invoked from recursive directory watching or not watching directories that are excluded on os that don't support recursive watching (like linux)
  • 0a8d84f handles the server host configuration and external/inferred projects watch options with ignore.
  • 496939d reloading projects, reloads project from scratch
  • cd18da8 file updates when reloading are reflected

This provides a way on systems where watching can be expensive and letting user refresh things if something changes rather than us having to poll or rely on events
Fixes #33335, #36035, #36243, #36394

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@sheetalkamat
Copy link
Member Author

This is now ready for review

@amcasey
Copy link
Member

amcasey commented Jun 25, 2020

letting user refresh things

Remind me how the editor does this? Is there a protocol refresh to re-poll the disk?

os that don't support recursive watching (like linux)

Does that mean Windows and Mac do support recursive watching?

@sheetalkamat
Copy link
Member Author

Remind me how the editor does this? Is there a protocol refresh to re-poll the disk?

Restart tsserver from vscode.. Dont know about vs command for that

Does that mean Windows and Mac do support recursive watching?

Yes: https://github.com/microsoft/TypeScript/blob/master/src/compiler/sys.ts#L1163

@amcasey
Copy link
Member

amcasey commented Jun 26, 2020

Remind me how the editor does this? Is there a protocol refresh to re-poll the disk?

Restart tsserver from vscode.. Dont know about vs command for that

That seems like overkill. Wouldn't there be a shorter delay if we just repolled all the files and then did an updateGraph? The user scenario I have in mind is hitting refresh in the file list tool window (aka the Solution Explorer in VS).

@larose
Copy link

larose commented Jul 6, 2020

Thanks for this PR @sheetalkamat. I really look forward for this PR to be merged as VS Code/TypeScript is unfortunately too often unresponsive on Linux when I'm working on one of my project that has about 2000 packages in node_modules. Let me know if I can do anything to help you.

@controlrepo
Copy link

That'd be amazing to have this one resolved finally. It has been first reported almost 2 years ago and there's little progress.

   INOTIFY
   WATCHER
    COUNT     PID     CMD
----------------------------------------
  503994     4631  /usr/share/code/code /usr/share/code/resources/app/out/bootstrap-fork --type=watcherService
   20038     4631  /usr/share/code/code /usr/share/code/resources/app/out/bootstrap-fork --type=watcherService

That's quite extensive for a

(Fedora31, vsc 1.47.2)
(and I don't even have any node_modules folder, and tried it all to ignore everything and unignore only very few folders which disrupted my experience with ag, but to no avail. It's ridiculous).

@sheetalkamat
Copy link
Member Author

@amcasey i have updated the PR per your suggestion so that reload projects, loads the project from scratch

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

This is a surprisingly large change and I'm still going through some things, but here's my first round of comments.

src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
src/compiler/commandLineParser.ts Show resolved Hide resolved
src/compiler/commandLineParser.ts Show resolved Hide resolved
src/compiler/commandLineParser.ts Show resolved Hide resolved
src/compiler/commandLineParser.ts Show resolved Hide resolved
src/compiler/sys.ts Show resolved Hide resolved
src/compiler/sys.ts Show resolved Hide resolved
src/compiler/watchPublic.ts Show resolved Hide resolved
src/server/editorServices.ts Show resolved Hide resolved
@amcasey
Copy link
Member

amcasey commented Sep 5, 2020

@mjbvz Could you possibly confirm that the new project reload functionality behaves sensibly in VS Code? I don't believe VS uses it.

@sheetalkamat
Copy link
Member Author

@amcasey @mjbvz i would like to get this in for 4.2
Can you please review

Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Protocol changes look good to me.

Do you think we should add VS Code settings to control this too?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

I like the concept, I have some quibbles with the implementation, and I have one remaining concern: can I disable all file watching and just hit "refresh" in the editor to trigger it when I need it?

src/compiler/commandLineParser.ts Show resolved Hide resolved
src/compiler/sys.ts Show resolved Hide resolved
src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
@sheetalkamat sheetalkamat merged commit 76cf8fd into master Nov 4, 2020
@sheetalkamat sheetalkamat deleted the watchIgnore branch November 4, 2020 21:30
@Utsav2
Copy link

Utsav2 commented Apr 5, 2021

Are there any docs for this feature? Also, does this automatically take the include and exclude dirs from tsconfig.json? it seems unintuitive that the watch is set up on files ignored by tsc

@sheetalkamat
Copy link
Member Author

By default nothing is ignored. User has to opt for directories/files to ignore..
@orta i think this is not in tsconfig reference section yet.

@orta
Copy link
Contributor

orta commented Apr 9, 2021

Thanks sheetal - looks like I manually re-created a partial set of the watch flags back when I made the tsconfig reference, will move it to be automatic to pick these up!

@Kieran-Lynn
Copy link

Kieran-Lynn commented Apr 17, 2021

@orta @sheetalkamat I cant seem to find any docs on this.

https://www.typescriptlang.org/tsconfig#Watch_Options_999

This has nothing about the new fields that are supposed to be added. Im trying to add

"watchOptions": { "excludeDirectories" : ["node_modules"] }

to my tsconfig but Im still getting extremely slow load times on my project when using vscode in WSL with files on the C drive

@sheetalkamat
Copy link
Member Author

@Kierchon it also needs to be set in vscode preferences but i dont find it in vscode preferences in watchOptions
@mjbvz did we miss adding excludeDirectories and excludeFiles as watchOption in vscode preferences?

@mjbvz
Copy link
Contributor

mjbvz commented Apr 20, 2021

You should just be able to add them in the settings.json:

  "typescript.tsserver.watchOptions": {
    "watchFile": "useFsEvents",
    "excludeDirectories": []
  },

We pass the entire object on the TS Server

@sheetalkamat Should these settings also be documented? If so, can you please share a quick blurb about what these two settings do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
9 participants