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.

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

Watch events enhancements #57950

Merged
merged 4 commits into from
Mar 27, 2024
Merged

Watch events enhancements #57950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f58ab7
Add test for node_modules file
sheetalkamat Mar 26, 2024
ddbd1ce
Dont watch for update events if not watching node_modules directory
sheetalkamat Mar 26, 2024
d1dcc7a
Change protocol to send events together for watchChange
sheetalkamat Mar 27, 2024
7b998fa
API baseline
sheetalkamat Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 30 additions & 26 deletions src/server/editorServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,16 @@ function createWatchFactoryHostUsingWatchEvents(service: ProjectService, canUseW
recursive ? watchedDirectoriesRecursive : watchedDirectories,
path,
callback,
id => ({ eventName: CreateDirectoryWatcherEvent, data: { id, path, recursive: !!recursive } }),
id => ({
eventName: CreateDirectoryWatcherEvent,
data: {
id,
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,
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member

@bpasero bpasero Mar 27, 2024 •
edited

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.

Copy link
Member Author

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.

},
}),
);
}
function getOrCreateFileWatcher<T>(
Expand Down Expand Up @@ -963,37 +972,32 @@ function createWatchFactoryHostUsingWatchEvents(service: ProjectService, canUseW
},
};
}
function onWatchChange({ id, path, eventType }: protocol.WatchChangeRequestArgs) {
// console.log(`typescript-vscode-watcher:: Invoke:: ${id}:: ${path}:: ${eventType}`);
onFileWatcherCallback(id, path, eventType);
onDirectoryWatcherCallback(watchedDirectories, id, path, eventType);
onDirectoryWatcherCallback(watchedDirectoriesRecursive, id, path, eventType);
function onWatchChange(args: protocol.WatchChangeRequestArgs | readonly protocol.WatchChangeRequestArgs[]) {
if (isArray(args)) args.forEach(onWatchChangeRequestArgs);
else onWatchChangeRequestArgs(args);
}

function onFileWatcherCallback(
id: number,
eventPath: string,
eventType: "create" | "delete" | "update",
) {
watchedFiles.idToCallbacks.get(id)?.forEach(callback => {
const eventKind = eventType === "create" ?
FileWatcherEventKind.Created :
eventType === "delete" ?
FileWatcherEventKind.Deleted :
FileWatcherEventKind.Changed;
callback(eventPath, eventKind);
});
function onWatchChangeRequestArgs({ id, created, deleted, updated }: protocol.WatchChangeRequestArgs) {
onWatchEventType(id, created, FileWatcherEventKind.Created);
onWatchEventType(id, deleted, FileWatcherEventKind.Deleted);
onWatchEventType(id, updated, FileWatcherEventKind.Changed);
}

function onWatchEventType(id: number, paths: readonly string[] | undefined, eventKind: FileWatcherEventKind) {
if (!paths?.length) return;
forEachCallback(watchedFiles, id, paths, (callback, eventPath) => callback(eventPath, eventKind));
forEachCallback(watchedDirectories, id, paths, (callback, eventPath) => callback(eventPath));
forEachCallback(watchedDirectoriesRecursive, id, paths, (callback, eventPath) => callback(eventPath));
}

function onDirectoryWatcherCallback(
{ idToCallbacks }: HostWatcherMap<DirectoryWatcherCallback>,
function forEachCallback<T>(
hostWatcherMap: HostWatcherMap<T>,
id: number,
eventPath: string,
eventType: "create" | "delete" | "update",
eventPaths: readonly string[],
cb: (callback: T, eventPath: string) => void,
) {
if (eventType === "update") return;
idToCallbacks.get(id)?.forEach(callback => {
callback(eventPath);
hostWatcherMap.idToCallbacks.get(id)?.forEach(callback => {
eventPaths.forEach(eventPath => cb(callback, eventPath));
});
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1849,13 +1849,13 @@ export interface CloseRequest extends FileRequest {

export interface WatchChangeRequest extends Request {
command: CommandTypes.WatchChange;
arguments: WatchChangeRequestArgs;
arguments: WatchChangeRequestArgs | readonly WatchChangeRequestArgs[];
}

export interface WatchChangeRequestArgs {
id: number;
path: string;
eventType: "create" | "delete" | "update";
created?: string[];
deleted?: string[];
updated?: string[];
}

/**
Expand Down Expand Up @@ -2656,6 +2656,7 @@ export interface CreateDirectoryWatcherEventBody {
readonly id: number;
readonly path: string;
readonly recursive: boolean;
readonly ignoreUpdate?: boolean;
}

export type CloseFileWatcherEventName = "closeFileWatcher";
Expand Down
172 changes: 148 additions & 24 deletions src/testRunner/unittests/tsserver/events/watchEvents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
} from "../../../../harness/tsserverLogger";
import {
createWatchUtils,
Watches,
WatchUtils,
} from "../../../../harness/watchUtils";
import * as ts from "../../../_namespaces/ts";
Expand Down Expand Up @@ -60,11 +61,11 @@ describe("unittests:: tsserver:: events:: watchEvents", () => {
}

function watchDirectory(data: ts.server.protocol.CreateDirectoryWatcherEventBody) {
logger.log(`Custom watchDirectory: ${data.id}: ${data.path} ${data.recursive}`);
logger.log(`Custom watchDirectory: ${data.id}: ${data.path} ${data.recursive} ${data.ignoreUpdate}`);
ts.Debug.assert(!idToClose.has(data.id));
const result = host.factoryData.watchUtils.fsWatch(data.path, data.recursive, data);
idToClose.set(data.id, () => {
logger.log(`Custom watchDirectory:: Close:: ${data.id}: ${data.path} ${data.recursive}`);
logger.log(`Custom watchDirectory:: Close:: ${data.id}: ${data.path} ${data.recursive} ${data.ignoreUpdate}`);
result.close();
});
}
Expand All @@ -89,37 +90,132 @@ describe("unittests:: tsserver:: events:: watchEvents", () => {
}
}

function updateFileOnHost(session: TestSession, file: string, log: string) {
function updateFileOnHost(session: TestSession, file: string, log: string, content?: string) {
// Change b.ts
session.logger.log(log);
session.host.writeFile(file, session.host.readFile("/user/username/projects/myproject/a.ts")!);
session.logger.log(`${log}: ${file}`);
if (content) session.host.appendFile(file, content);
else session.host.writeFile(file, session.host.readFile("/user/username/projects/myproject/a.ts")!);
session.host.runQueuedTimeoutCallbacks();
}

function collectWatchChanges<T extends ts.server.protocol.CreateFileWatcherEventBody | ts.server.protocol.CreateDirectoryWatcherEventBody>(
session: TestSession,
watches: Watches<T>,
path: string,
eventPath: string,
eventType: "created" | "deleted" | "updated",
ignoreUpdate?: (data: T) => boolean,
) {
session.logger.log(`Custom watch:: ${path} ${eventPath} ${eventType}`);
let result: ts.server.protocol.WatchChangeRequestArgs[] | undefined;
watches.forEach(
path,
data => {
if (ignoreUpdate?.(data)) return;
switch (eventType) {
case "created":
(result ??= []).push({ id: data.id, created: [eventPath] });
break;
case "deleted":
(result ??= []).push({ id: data.id, deleted: [eventPath] });
break;
case "updated":
(result ??= []).push({ id: data.id, updated: [eventPath] });
break;
default:
ts.Debug.assertNever(eventType);
}
},
);
return result;
}

function collectDirectoryWatcherChanges(
session: TestSession,
dir: string,
eventPath: string,
eventType: "created" | "deleted" | "updated",
) {
return collectWatchChanges(
session,
(session.logger.host as TestServerHostWithCustomWatch).factoryData.watchUtils.fsWatchesRecursive,
dir,
eventPath,
eventType,
data => !!data.ignoreUpdate && eventType === "updated",
);
}

function collectFileWatcherChanges(
session: TestSession,
file: string,
eventType: "created" | "deleted" | "updated",
) {
return collectWatchChanges(
session,
(session.logger.host as TestServerHostWithCustomWatch).factoryData.watchUtils.pollingWatches,
file,
file,
eventType,
);
}

function invokeWatchChange(
session: TestSession,
...args: (ts.server.protocol.WatchChangeRequestArgs[] | undefined)[]
) {
let result: Map<number, ts.server.protocol.WatchChangeRequestArgs> | undefined;
args.forEach(arg =>
arg?.forEach(value => {
result ??= new Map();
const valueInResult = result.get(value.id);
if (!valueInResult) result.set(value.id, value);
else {
valueInResult.created = ts.combine(valueInResult.created, value.created);
valueInResult.deleted = ts.combine(valueInResult.deleted, value.deleted);
valueInResult.updated = ts.combine(valueInResult.updated, value.updated);
}
})
);
if (result) {
session.executeCommandSeq<ts.server.protocol.WatchChangeRequest>({
command: ts.server.protocol.CommandTypes.WatchChange,
arguments: ts.singleOrMany(ts.arrayFrom(result.values())),
});
}
}

function addFile(session: TestSession, path: string) {
updateFileOnHost(session, path, "Add file");
session.logger.log("Custom watch");
(session.logger.host as TestServerHostWithCustomWatch).factoryData.watchUtils.fsWatchesRecursive.forEach(
"/user/username/projects/myproject",
data =>
session.executeCommandSeq<ts.server.protocol.WatchChangeRequest>({
command: ts.server.protocol.CommandTypes.WatchChange,
arguments: { id: data.id, path, eventType: "create" },
}),
invokeWatchChange(
session,
collectDirectoryWatcherChanges(session, "/user/username/projects/myproject", path, "created"),
);
session.host.runQueuedTimeoutCallbacks();
}

function changeFile(session: TestSession, path: string) {
updateFileOnHost(session, path, "Change File");
session.logger.log("Custom watch");
(session.logger.host as TestServerHostWithCustomWatch).factoryData.watchUtils.pollingWatches.forEach(
path,
data =>
session.executeCommandSeq<ts.server.protocol.WatchChangeRequest>({
command: ts.server.protocol.CommandTypes.WatchChange,
arguments: { id: data.id, path, eventType: "update" },
}),
function changeFile(session: TestSession, path: string, content?: string) {
updateFileOnHost(session, path, "Change File", content);
invokeWatchChange(
session,
collectFileWatcherChanges(session, path, "updated"),
collectDirectoryWatcherChanges(session, ts.getDirectoryPath(path), path, "updated"),
);
session.host.runQueuedTimeoutCallbacks();
}

function npmInstall(session: TestSession) {
session.logger.log("update with npm install");
session.host.appendFile("/user/username/projects/myproject/node_modules/something/index.d.ts", `export const y = 20;`);
session.host.runQueuedTimeoutCallbacks();
invokeWatchChange(
session,
collectDirectoryWatcherChanges(
session,
"/user/username/projects/myproject/node_modules",
"/user/username/projects/myproject/node_modules/something/index.d.ts",
"updated",
),
);
session.host.runQueuedTimeoutCallbacks();
}
Expand All @@ -129,6 +225,8 @@ describe("unittests:: tsserver:: events:: watchEvents", () => {
"/user/username/projects/myproject/tsconfig.json": "{}",
"/user/username/projects/myproject/a.ts": `export class a { prop = "hello"; foo() { return this.prop; } }`,
"/user/username/projects/myproject/b.ts": `export class b { prop = "hello"; foo() { return this.prop; } }`,
"/user/username/projects/myproject/m.ts": `import { x } from "something"`,
"/user/username/projects/myproject/node_modules/something/index.d.ts": `export const x = 10;`,
[libFile.path]: libFile.content,
});
const logger = createLoggerWithInMemoryLogs(inputHost);
Expand All @@ -153,6 +251,26 @@ describe("unittests:: tsserver:: events:: watchEvents", () => {
// Re watch
closeFilesForSession(["/user/username/projects/myproject/b.ts"], session);

// Update c.ts
changeFile(session, "/user/username/projects/myproject/c.ts", `export const y = 20;`);

// Update with npm install
npmInstall(session);
host.runQueuedTimeoutCallbacks();

// Add and change multiple files - combine and send multiple requests together
updateFileOnHost(session, "/user/username/projects/myproject/d.ts", "Add file");
updateFileOnHost(session, "/user/username/projects/myproject/c.ts", "Change File", `export const z = 30;`);
updateFileOnHost(session, "/user/username/projects/myproject/e.ts", "Add File");
invokeWatchChange(
session,
collectDirectoryWatcherChanges(session, "/user/username/projects/myproject", "/user/username/projects/myproject/d.ts", "created"),
collectFileWatcherChanges(session, "/user/username/projects/myproject/c.ts", "updated"),
collectDirectoryWatcherChanges(session, "/user/username/projects/myproject", "/user/username/projects/myproject/c.ts", "updated"),
collectDirectoryWatcherChanges(session, "/user/username/projects/myproject", "/user/username/projects/myproject/e.ts", "created"),
);
session.host.runQueuedTimeoutCallbacks();

baselineTsserverLogs("events/watchEvents", `canUseWatchEvents`, session);
function handleWatchEvents(event: ts.server.ProjectServiceEvent) {
switch (event.eventName) {
Expand Down Expand Up @@ -192,9 +310,15 @@ describe("unittests:: tsserver:: events:: watchEvents", () => {
logger.msg = (s, type) => logger.info(`${type}:: ${s}`);
session.executeCommandSeq<ts.server.protocol.WatchChangeRequest>({
command: ts.server.protocol.CommandTypes.WatchChange,
arguments: { id: 1, path: "/user/username/projects/myproject/b.ts", eventType: "update" },
arguments: { id: 1, updated: ["/user/username/projects/myproject/b.ts"] },
});

// Update c.ts
changeFile(session, "/user/username/projects/myproject/c.ts", `export const y = 20;`);

// Update with npm install
npmInstall(session);

baselineTsserverLogs("events/watchEvents", `canUseWatchEvents without canUseEvents`, session);
});
});
8 changes: 5 additions & 3 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,12 +1342,13 @@ declare namespace ts {
}
export interface WatchChangeRequest extends Request {
command: CommandTypes.WatchChange;
arguments: WatchChangeRequestArgs;
arguments: WatchChangeRequestArgs | readonly WatchChangeRequestArgs[];
}
export interface WatchChangeRequestArgs {
id: number;
path: string;
eventType: "create" | "delete" | "update";
created?: string[];
deleted?: string[];
updated?: string[];
}
/**
* Request to obtain the list of files that should be regenerated if target file is recompiled.
Expand Down Expand Up @@ -2032,6 +2033,7 @@ declare namespace ts {
readonly id: number;
readonly path: string;
readonly recursive: boolean;
readonly ignoreUpdate?: boolean;
}
export type CloseFileWatcherEventName = "closeFileWatcher";
export interface CloseFileWatcherEvent extends Event {
Expand Down