-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
Watch events enhancements #57950
Watch events enhancements #57950
Conversation
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, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary. |
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
path, | ||
recursive: !!recursive, | ||
// Special case node_modules as we watch it for changes to closed script infos as well | ||
ignoreUpdate: !path.endsWith("/node_modules") ? true : undefined, |
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.
Can this path be \node_modules
on Windows?
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.
Slightly related: I did notice another watch request for node_modules/@types
, so I wonder if these could also ignoreUpdate
?
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.
it isnt "\node_module" on windows because our paths are normalized.
We dont care about updates in node_modules/@types
since thats not what we watch for file updates. We only watch node_modules
for updates to scriptinfos that are anywhere including in @types
so this condition suffices
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.
Hm, I think I misunderstood our discussion around changes in node_modules
then. I had assumed TS only cares about added/deleted events in node_modules
to rerun module resolution, but not for updates. So now with your change, folder watchers ignore updates unless its node_modules
being watched?
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.
Yes. we only care about updates
inside node_modules
and no other folder.
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.
Reference to code here https://github.com/microsoft/TypeScript/blob/main/src/server/editorServices.ts#L3102 this normally works out because with fs.watch most of the time we get "rename" event instead of "change" when doing npm i
but with parcel you get update
so that cannot be ignored from vscode watcher usage perspective
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.
Yeah I understand. My idea was to collect events to node_modules
in the TS extension and then send you a single event per module that is impacted to the server. That can be a added
event to emulate the rename
you got from fs.watch
. Its a bit of a hack and very specific to this case but would essentially reduce the event load on node_modules
, which can be a huge source of file events. I do not have strong feelings though, now with the batching it might not make much of a difference.
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.
What i meant by ignoreUpdate
here is that we really dont care if there is file that was modified within that folder and thats true for any folder watched except node_modules
. So from protocol wise this change looks good.
On whether to send single directory event
for module
inside node_modules
or all files can be totally customized irrespective of this protocol change. In general, the more specific you are about the path
the better reuse happens between programs. We were anyways getting these events from fs.watch
and we handle both cases since the fileName may or may not be present when the event comes through fs.watch
.
@typescript-bot cherry-pick this to release-5.4 |
Hey, @DanielRosenwasser! I've created #57967 for you. This involved updating baselines; please check the diff. |
Vscode PR reference: microsoft/vscode#193848
cc: @bpasero