From ff25f3b86de10eb270ee13b4414f2a782f1f838d Mon Sep 17 00:00:00 2001 From: "Lyu, Wei Da" Date: Wed, 26 Jun 2024 14:36:56 +0800 Subject: [PATCH 1/7] fix: handle issues with svelte file watching --- .../src/lib/documents/DocumentManager.ts | 3 +- .../plugins/typescript/LSAndTSDocResolver.ts | 14 ++++- .../plugins/typescript/TypeScriptPlugin.ts | 4 +- .../src/plugins/typescript/service.ts | 8 ++- .../typescript/TypescriptPlugin.test.ts | 32 +++++++++- .../test/plugins/typescript/service.test.ts | 59 +++++++++++++++++++ 6 files changed, 110 insertions(+), 10 deletions(-) diff --git a/packages/language-server/src/lib/documents/DocumentManager.ts b/packages/language-server/src/lib/documents/DocumentManager.ts index 048219717..acc5c58c1 100644 --- a/packages/language-server/src/lib/documents/DocumentManager.ts +++ b/packages/language-server/src/lib/documents/DocumentManager.ts @@ -47,7 +47,8 @@ export class DocumentManager { let document: Document; if (this.documents.has(textDocument.uri)) { document = this.documents.get(textDocument.uri)!; - document.openedByClient = openedByClient; + // open state should only be update when the document is closed + document.openedByClient ||= openedByClient; document.setText(textDocument.text); } else { document = this.createDocument(textDocument); diff --git a/packages/language-server/src/plugins/typescript/LSAndTSDocResolver.ts b/packages/language-server/src/plugins/typescript/LSAndTSDocResolver.ts index f3b8d6f48..3843e8a89 100644 --- a/packages/language-server/src/plugins/typescript/LSAndTSDocResolver.ts +++ b/packages/language-server/src/plugins/typescript/LSAndTSDocResolver.ts @@ -204,11 +204,21 @@ export class LSAndTSDocResolver { * Updates snapshot path in all existing ts services and retrieves snapshot */ async updateSnapshotPath(oldPath: string, newPath: string): Promise { + const document = this.docManager.get(pathToUrl(oldPath)); + const isOpenedInClient = document?.openedByClient; for (const snapshot of this.globalSnapshotsManager.getByPrefix(oldPath)) { await this.deleteSnapshot(snapshot.filePath); } - // This may not be a file but a directory, still try - await this.getSnapshot(newPath); + + if (isOpenedInClient) { + this.docManager.openClientDocument({ + uri: pathToUrl(newPath), + text: document!.getText() + }); + } else { + // This may not be a file but a directory, still try + await this.getSnapshot(newPath); + } } /** diff --git a/packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts b/packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts index 5ce566ade..b052688e9 100644 --- a/packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts +++ b/packages/language-server/src/plugins/typescript/TypeScriptPlugin.ts @@ -35,7 +35,7 @@ import { mapSymbolInformationToOriginal } from '../../lib/documents'; import { LSConfigManager, LSTypescriptConfig } from '../../ls-config'; -import { isNotNullOrUndefined, isZeroLengthRange } from '../../utils'; +import { isNotNullOrUndefined, isZeroLengthRange, pathToUrl } from '../../utils'; import { AppCompletionItem, AppCompletionList, @@ -523,7 +523,7 @@ export class TypeScriptPlugin const isSvelteFile = isSvelteFilePath(fileName); const isClientSvelteFile = - isSvelteFile && this.documentManager.get(fileName)?.openedByClient; + isSvelteFile && this.documentManager.get(pathToUrl(fileName))?.openedByClient; if (changeType === FileChangeType.Deleted) { if (!isClientSvelteFile) { diff --git a/packages/language-server/src/plugins/typescript/service.ts b/packages/language-server/src/plugins/typescript/service.ts index f0471e78c..fa2fe0c2b 100644 --- a/packages/language-server/src/plugins/typescript/service.ts +++ b/packages/language-server/src/plugins/typescript/service.ts @@ -353,11 +353,15 @@ async function createLanguageService( return; } + const canonicalWorkspacePath = getCanonicalFileName(workspacePath); const patterns: RelativePattern[] = []; Object.entries(wildcardDirectories).forEach(([dir, flags]) => { - // already watched - if (getCanonicalFileName(dir).startsWith(workspacePath)) { + if ( + // already watched + getCanonicalFileName(dir).startsWith(canonicalWorkspacePath) || + !tsSystem.directoryExists(dir) + ) { return; } patterns.push({ diff --git a/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts b/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts index 286c15b44..2323e429f 100644 --- a/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts +++ b/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts @@ -57,7 +57,7 @@ describe('TypescriptPlugin', function () { docManager ); docManager.openClientDocument('some doc'); - return { plugin, document, lsAndTsDocResolver }; + return { plugin, document, lsAndTsDocResolver, docManager }; } it('provides document symbols', async () => { @@ -618,7 +618,7 @@ describe('TypescriptPlugin', function () { }); const setupForOnWatchedFileChanges = async () => { - const { plugin, document, lsAndTsDocResolver } = setup('empty.svelte'); + const { plugin, document, lsAndTsDocResolver, docManager } = setup('empty.svelte'); const targetSvelteFile = document.getFilePath()!; const snapshotManager = (await lsAndTsDocResolver.getTSService(targetSvelteFile)) .snapshotManager; @@ -627,7 +627,8 @@ describe('TypescriptPlugin', function () { snapshotManager, plugin, targetSvelteFile, - lsAndTsDocResolver + lsAndTsDocResolver, + docManager }; }; @@ -769,6 +770,31 @@ describe('TypescriptPlugin', function () { ); }); + it.only("shouldn't close svelte document when renamed", async () => { + const { plugin, docManager, targetSvelteFile } = await setupForOnWatchedFileChanges(); + docManager.openClientDocument({ + text: '', + uri: pathToUrl(targetSvelteFile) + }); + + const basename = path.basename(targetSvelteFile); + const newFileName = basename.replace('.svelte', '').toUpperCase() + '.svelte'; + const newFilePath = path.join(path.dirname(targetSvelteFile), newFileName); + await plugin.onWatchFileChanges([ + { + fileName: targetSvelteFile, + changeType: FileChangeType.Deleted + }, + { + fileName: newFilePath, + changeType: FileChangeType.Created + } + ]); + + const document = docManager.get(pathToUrl(targetSvelteFile)); + assert.ok(document); + }); + // Hacky, but it works. Needed due to testing both new and old transformation after(() => { __resetCache(); diff --git a/packages/language-server/test/plugins/typescript/service.test.ts b/packages/language-server/test/plugins/typescript/service.test.ts index 8d3ee0efe..02cac2d21 100644 --- a/packages/language-server/test/plugins/typescript/service.test.ts +++ b/packages/language-server/test/plugins/typescript/service.test.ts @@ -9,6 +9,8 @@ import { } from '../../../src/plugins/typescript/service'; import { pathToUrl } from '../../../src/utils'; import { createVirtualTsSystem, getRandomVirtualDirPath } from './test-utils'; +import sinon from 'sinon'; +import { RelativePattern } from 'vscode-languageserver-protocol'; describe('service', () => { const testDir = path.join(__dirname, 'testfiles'); @@ -263,4 +265,61 @@ describe('service', () => { ls.getService().getSemanticDiagnostics(document.getFilePath()!); }); }); + + it('skip directory watching if directory is root', async () => { + const dirPath = getRandomVirtualDirPath(path.join(testDir, 'Test')); + const { virtualSystem, lsDocumentContext } = setup(); + + const rootUris = [pathToUrl(dirPath)]; + + const watchDirectory = sinon.spy(); + lsDocumentContext.watchDirectory = watchDirectory; + lsDocumentContext.nonRecursiveWatchPattern = '*.ts'; + + virtualSystem.readDirectory = () => []; + virtualSystem.directoryExists = () => true; + + virtualSystem.writeFile( + path.join(dirPath, 'tsconfig.json'), + JSON.stringify({ + compilerOptions: {}, + include: ['src/**/*.ts', 'test/**/*.ts', '../foo/**/*.ts'] + }) + ); + + await getService(path.join(dirPath, 'random.svelte'), rootUris, lsDocumentContext); + + sinon.assert.calledWith(watchDirectory.firstCall, [ + { + baseUri: pathToUrl(path.join(dirPath, '../foo')), + pattern: '**/*.ts' + } + ]); + }); + + it('skip directory watching if directory do not exist', async () => { + const dirPath = getRandomVirtualDirPath(path.join(testDir, 'Test')); + const { virtualSystem, lsDocumentContext } = setup(); + + const rootUris = [pathToUrl(dirPath)]; + + const watchDirectory = sinon.spy(); + lsDocumentContext.watchDirectory = watchDirectory; + lsDocumentContext.nonRecursiveWatchPattern = '*.ts'; + + virtualSystem.readDirectory = () => []; + virtualSystem.directoryExists = () => false; + + virtualSystem.writeFile( + path.join(dirPath, 'tsconfig.json'), + JSON.stringify({ + compilerOptions: {}, + include: ['../test/**/*.ts'] + }) + ); + + await getService(path.join(dirPath, 'random.svelte'), rootUris, lsDocumentContext); + + sinon.assert.calledWith(watchDirectory.firstCall, []); + }); }); From 87c35581ff5d6cd797c3cc252224d2060023b0d2 Mon Sep 17 00:00:00 2001 From: "Lyu, Wei Da" Date: Wed, 26 Jun 2024 14:37:26 +0800 Subject: [PATCH 2/7] does this happens in ci? --- .../language-server/src/lib/documents/DocumentManager.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/language-server/src/lib/documents/DocumentManager.ts b/packages/language-server/src/lib/documents/DocumentManager.ts index acc5c58c1..459c6282f 100644 --- a/packages/language-server/src/lib/documents/DocumentManager.ts +++ b/packages/language-server/src/lib/documents/DocumentManager.ts @@ -47,7 +47,10 @@ export class DocumentManager { let document: Document; if (this.documents.has(textDocument.uri)) { document = this.documents.get(textDocument.uri)!; - // open state should only be update when the document is closed + // open state should only be update when the document is closed + if (document.openedByClient && !openedByClient) { + console.trace('why is this happening?') + } document.openedByClient ||= openedByClient; document.setText(textDocument.text); } else { From 383c29449a92843044201a75c9d23dc14615b833 Mon Sep 17 00:00:00 2001 From: "Lyu, Wei Da" Date: Wed, 26 Jun 2024 14:38:08 +0800 Subject: [PATCH 3/7] oops --- .../test/plugins/typescript/TypescriptPlugin.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts b/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts index 2323e429f..671897804 100644 --- a/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts +++ b/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts @@ -770,7 +770,7 @@ describe('TypescriptPlugin', function () { ); }); - it.only("shouldn't close svelte document when renamed", async () => { + it("shouldn't close svelte document when renamed", async () => { const { plugin, docManager, targetSvelteFile } = await setupForOnWatchedFileChanges(); docManager.openClientDocument({ text: '', From f9269bf7b8e737ea9005fcf9ccc5bb5fa6f4c0ea Mon Sep 17 00:00:00 2001 From: "Lyu, Wei Da" Date: Wed, 26 Jun 2024 15:30:43 +0800 Subject: [PATCH 4/7] revert --- .../language-server/src/lib/documents/DocumentManager.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/language-server/src/lib/documents/DocumentManager.ts b/packages/language-server/src/lib/documents/DocumentManager.ts index 459c6282f..acc5c58c1 100644 --- a/packages/language-server/src/lib/documents/DocumentManager.ts +++ b/packages/language-server/src/lib/documents/DocumentManager.ts @@ -47,10 +47,7 @@ export class DocumentManager { let document: Document; if (this.documents.has(textDocument.uri)) { document = this.documents.get(textDocument.uri)!; - // open state should only be update when the document is closed - if (document.openedByClient && !openedByClient) { - console.trace('why is this happening?') - } + // open state should only be update when the document is closed document.openedByClient ||= openedByClient; document.setText(textDocument.text); } else { From 44c9c0508b213d8c41b6261af75721b3a4fdc1bb Mon Sep 17 00:00:00 2001 From: "Lyu, Wei Da" Date: Wed, 26 Jun 2024 16:04:17 +0800 Subject: [PATCH 5/7] one more test --- .../typescript/TypescriptPlugin.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts b/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts index 671897804..d92d452b2 100644 --- a/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts +++ b/packages/language-server/test/plugins/typescript/TypescriptPlugin.test.ts @@ -795,6 +795,24 @@ describe('TypescriptPlugin', function () { assert.ok(document); }); + it("shouldn't mark client svelte document as close", async () => { + const { plugin, docManager, targetSvelteFile } = await setupForOnWatchedFileChanges(); + docManager.openClientDocument({ + text: '', + uri: pathToUrl(targetSvelteFile) + }); + + await plugin.onWatchFileChanges([ + { + fileName: targetSvelteFile, + changeType: FileChangeType.Changed + } + ]); + + const document = docManager.get(pathToUrl(targetSvelteFile)); + assert.equal(document?.openedByClient, true); + }); + // Hacky, but it works. Needed due to testing both new and old transformation after(() => { __resetCache(); From 1270c3e97d2369317ea8b67fa8a7e144f68e9c38 Mon Sep 17 00:00:00 2001 From: "Lyu, Wei Da" Date: Wed, 26 Jun 2024 16:10:50 +0800 Subject: [PATCH 6/7] grammar --- packages/language-server/src/lib/documents/DocumentManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/language-server/src/lib/documents/DocumentManager.ts b/packages/language-server/src/lib/documents/DocumentManager.ts index acc5c58c1..1c2df541b 100644 --- a/packages/language-server/src/lib/documents/DocumentManager.ts +++ b/packages/language-server/src/lib/documents/DocumentManager.ts @@ -47,7 +47,7 @@ export class DocumentManager { let document: Document; if (this.documents.has(textDocument.uri)) { document = this.documents.get(textDocument.uri)!; - // open state should only be update when the document is closed + // open state should only be updated when the document is closed document.openedByClient ||= openedByClient; document.setText(textDocument.text); } else { From 8046cf04c488eb5081068d526dea5aaf2a3aa35b Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Wed, 26 Jun 2024 11:36:30 +0200 Subject: [PATCH 7/7] comments --- packages/language-server/src/server.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/language-server/src/server.ts b/packages/language-server/src/server.ts index 632681f43..86a1c5836 100644 --- a/packages/language-server/src/server.ts +++ b/packages/language-server/src/server.ts @@ -104,6 +104,8 @@ export function startServer(options?: LSOptions) { pendingWatchPatterns = patterns; }; + // Include Svelte files to better deal with scenarios such as switching git branches + // where files that are not opened in the client could change const nonRecursiveWatchPattern = '*.{ts,js,mts,mjs,cjs,cts,json,svelte}'; const recursiveWatchPattern = '**/' + nonRecursiveWatchPattern; @@ -329,6 +331,8 @@ export function startServer(options?: LSOptions) { connection?.client.register(DidChangeWatchedFilesNotification.type, { watchers: [ { + // Editors have exlude configs, such as VSCode with `files.watcherExclude`, + // which means it's safe to watch recursively here globPattern: recursiveWatchPattern } ]