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

fix: watch svelte files and project files outside workspace #2299

Merged
merged 11 commits into from
Jun 24, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,16 @@ export class DocumentManager {
let document: Document;
if (this.documents.has(textDocument.uri)) {
document = this.documents.get(textDocument.uri)!;
document.openedByClient = openedByClient;
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
document.setText(textDocument.text);
} else {
document = this.createDocument(textDocument);
document.openedByClient = openedByClient;
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
this.documents.set(textDocument.uri, document);
this.notify('documentOpen', document);
}

this.notify('documentChange', document);
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
document.openedByClient = openedByClient;

return document;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,13 @@ export class LSAndTSDocResolver {
// where multiple files and their dependencies
// being loaded in a short period of times
docManager.on('documentOpen', (document) => {
const path = document.getFilePath();
const isNewFile = path && !this.globalSnapshotsManager.get(path);
handleDocumentChange(document);
if (isNewFile && document.openedByClient) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we would kick off the costly "update project files" logic on every new file created through the editor UI? Is there a way to do this in less cases? "update project files" reads the file system, creates a new program version etc, which seems costly.

Copy link
Member Author

@jasonlyu123 jasonlyu123 Jun 20, 2024

Choose a reason for hiding this comment

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

I changed to detection to file watch since #2393 also needs it. I also delay the check to the next time the language service is used. TypeScript also has some directory watching info in the config file parse result. So we can skip the project file check if the file isn't in the target directories. This should make it trigger a bit less. If there are performance regressions reported, there is a TypeScript's internal method to cache directory file entries and we can try using that.

The info is also used to watch directories outside the root using the fallback watch or when the client supports it. This is added because the optimization check might break a way to workaround directory watching outside the workspace, not sure if anyone actually uses it though 😅.

// check if is a file that belongs to other project as well
this.updateProjectFiles();
}
docManager.lockDocument(document.uri);
});

Expand Down
19 changes: 13 additions & 6 deletions packages/language-server/src/plugins/typescript/SnapshotManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,25 +102,32 @@ export class SnapshotManager {

private readonly watchExtensions = [
ts.Extension.Dts,
ts.Extension.Dcts,
ts.Extension.Dmts,
ts.Extension.Js,
ts.Extension.Cjs,
ts.Extension.Mjs,
ts.Extension.Jsx,
ts.Extension.Ts,
ts.Extension.Mts,
ts.Extension.Cts,
ts.Extension.Tsx,
ts.Extension.Json
ts.Extension.Json,
'.svelte'
];

constructor(
private globalSnapshotsManager: GlobalSnapshotsManager,
private fileSpec: TsFilesSpec,
private workspaceRoot: string,
projectFiles: string[],
useCaseSensitiveFileNames = ts.sys.useCaseSensitiveFileNames
private tsSystem: ts.System,
projectFiles: string[]
) {
this.onSnapshotChange = this.onSnapshotChange.bind(this);
this.globalSnapshotsManager.onChange(this.onSnapshotChange);
this.documents = new FileMap(useCaseSensitiveFileNames);
this.documents = new FileMap(tsSystem.useCaseSensitiveFileNames);
this.projectFileToOriginalCasing = new Map();
this.getCanonicalFileName = createGetCanonicalFileName(useCaseSensitiveFileNames);
this.getCanonicalFileName = createGetCanonicalFileName(tsSystem.useCaseSensitiveFileNames);

projectFiles.forEach((originalCasing) =>
this.projectFileToOriginalCasing.set(
Expand Down Expand Up @@ -153,7 +160,7 @@ export class SnapshotManager {
return;
}

const projectFiles = ts.sys
const projectFiles = this.tsSystem
.readDirectory(this.workspaceRoot, this.watchExtensions, exclude, include)
.map(normalizePath);

Expand Down
13 changes: 13 additions & 0 deletions packages/language-server/src/plugins/typescript/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ const configFileModifiedTime = new FileMap<Date | undefined>();
const configFileForOpenFiles = new FileMap<string>();
const pendingReloads = new FileSet();
const documentRegistries = new Map<string, ts.DocumentRegistry>();
const pendingForAllServices = new Set<Promise<void>>();

/**
* For testing only: Reset the cache for services.
Expand Down Expand Up @@ -158,6 +159,13 @@ export async function getService(
export async function forAllServices(
cb: (service: LanguageServiceContainer) => any
): Promise<void> {
const promise = forAllServicesWorker(cb);
pendingForAllServices.add(promise);
await promise;
pendingForAllServices.delete(promise);
}

async function forAllServicesWorker(cb: (service: LanguageServiceContainer) => any): Promise<void> {
for (const service of services.values()) {
cb(await service);
}
Expand Down Expand Up @@ -192,6 +200,10 @@ export async function getServiceForTsconfig(
service = await services.get(tsconfigPathOrWorkspacePath)!;
}

if (pendingForAllServices.size > 0) {
await Promise.all(pendingForAllServices);
}

return service;
}

Expand All @@ -215,6 +227,7 @@ async function createLanguageService(
docContext.globalSnapshotsManager,
raw,
workspacePath,
tsSystem,
files
);

Expand Down
81 changes: 79 additions & 2 deletions packages/language-server/test/plugins/typescript/service.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
import assert from 'assert';
import path from 'path';
import ts from 'typescript';
import { Document } from '../../../src/lib/documents';
import { Document, DocumentManager } from '../../../src/lib/documents';
import {
getService,
LanguageServiceDocumentContext
} from '../../../src/plugins/typescript/service';
import { GlobalSnapshotsManager } from '../../../src/plugins/typescript/SnapshotManager';
import { pathToUrl } from '../../../src/utils';
import { normalizePath, pathToUrl } from '../../../src/utils';
import { createVirtualTsSystem, getRandomVirtualDirPath } from './test-utils';
import { LSAndTSDocResolver } from '../../../src/plugins';
import { LSConfigManager } from '../../../src/ls-config';

describe('service', () => {
const testDir = path.join(__dirname, 'testfiles');
Expand Down Expand Up @@ -262,4 +264,79 @@ describe('service', () => {
ls.getService().getSemanticDiagnostics(document.getFilePath()!);
});
});

it('checks if newly opened document is in other projects', async () => {
const baseDirPath = getRandomVirtualDirPath(testDir);
const project1DirPath = path.join(baseDirPath, 'project1');
const project2DirPath = path.join(baseDirPath, 'project2');

const virtualSystem = createVirtualTsSystem(testDir);

const project1Files = [normalizePath(path.join(project1DirPath, 'file1.svelte'))];
const project2Files = [normalizePath(path.join(project2DirPath, 'file2.svelte'))];

// simple mock because adding virtual readDirectory requires logic for matching include/exclude patterns
virtualSystem.readDirectory = (path, _extensions, _exclude, _include, _depth) => {
const normalizedPath = normalizePath(path);
if (normalizedPath === normalizePath(project1DirPath)) {
return project1Files;
}

if (normalizedPath === normalizePath(project2DirPath)) {
return project2Files;
}

return [];
};

const rootUris = [pathToUrl(baseDirPath)];
const docManager = new DocumentManager(
(textDocument) => new Document(textDocument.uri, textDocument.text)
);
const lsConfigManager = new LSConfigManager();
const lsAndTSDocResolver = new LSAndTSDocResolver(docManager, rootUris, lsConfigManager, {
tsSystem: virtualSystem
});

virtualSystem.writeFile(
path.join(project1DirPath, 'tsconfig.json'),
JSON.stringify({
compilerOptions: <ts.CompilerOptions>{
checkJs: true,
strict: true
}
})
);

virtualSystem.writeFile(
path.join(project2DirPath, 'tsconfig.json'),
JSON.stringify({
compilerOptions: <ts.CompilerOptions>{
checkJs: true,
strict: true
},
include: ['../project1/**/*', '*.svelte']
})
);

await lsAndTSDocResolver.getLSForPath(path.join(project1DirPath, 'file1.svelte'));
await lsAndTSDocResolver.getLSForPath(path.join(project2DirPath, 'file2.svelte'));

const newFileInProject1 = normalizePath(path.join(project1DirPath, 'file3.svelte'));
project2Files.push(newFileInProject1);

docManager.openDocument(
{
uri: pathToUrl(newFileInProject1),
text: ''
},
true
);

const snapshotManagerForProject2 = await lsAndTSDocResolver.getSnapshotManager(
path.join(project2DirPath, 'file2.svelte')
);

assert.ok(snapshotManagerForProject2.getProjectFileNames().includes(newFileInProject1));
});
});
13 changes: 13 additions & 0 deletions packages/language-server/test/plugins/typescript/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,19 @@ export function createVirtualTsSystem(currentDirectory: string): ts.System {
},
getModifiedTime(path) {
return modifiedTime.get(normalizePath(toAbsolute(path)));
},
readDirectory(path, _extensions, _exclude, include, _depth) {
if (include && (include.length != 1 || include[0] !== '**/*')) {
throw new Error(
'include pattern matching not implemented. Mock it if the test needs it. Pattern: ' +
include
);
}

const normalizedPath = normalizePath(toAbsolute(path));
return Array.from(virtualFs.keys()).filter((fileName) =>
fileName.startsWith(normalizedPath)
);
}
};

Expand Down
Loading