From 840b213c73a5c6f3ac7e8ca5f2698e5cf93cafc0 Mon Sep 17 00:00:00 2001 From: Simon Date: Sun, 1 May 2022 22:15:31 +0200 Subject: [PATCH 1/3] (fix) prevent duplicate import updates The TS plugin and the Svelte extension want to update the same locations after a file move/rename in some cases; prevent that #641 --- .../features/UpdateImportsProvider.ts | 1 + packages/svelte-vscode/src/extension.ts | 14 ++++++-- packages/svelte-vscode/src/tsplugin.ts | 6 ++-- .../src/language-service/index.ts | 2 ++ .../src/language-service/update-imports.ts | 34 +++++++++++++++++++ 5 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 packages/typescript-plugin/src/language-service/update-imports.ts diff --git a/packages/language-server/src/plugins/typescript/features/UpdateImportsProvider.ts b/packages/language-server/src/plugins/typescript/features/UpdateImportsProvider.ts index 7349a450e..d9b3913fe 100644 --- a/packages/language-server/src/plugins/typescript/features/UpdateImportsProvider.ts +++ b/packages/language-server/src/plugins/typescript/features/UpdateImportsProvider.ts @@ -15,6 +15,7 @@ export class UpdateImportsProviderImpl implements UpdateImportsProvider { constructor(private readonly lsAndTsDocResolver: LSAndTSDocResolver) {} async updateImports(fileRename: FileRename): Promise { + // TODO does this handle folder moves/renames correctly? old/new path isn't a file then const oldPath = urlToPath(fileRename.oldUri); const newPath = urlToPath(fileRename.newUri); if (!oldPath || !newPath) { diff --git a/packages/svelte-vscode/src/extension.ts b/packages/svelte-vscode/src/extension.ts index ac29dab2c..9e9225a0e 100644 --- a/packages/svelte-vscode/src/extension.ts +++ b/packages/svelte-vscode/src/extension.ts @@ -352,13 +352,23 @@ function addRenameFileListener(getLS: () => LanguageClient) { newUri: evt.files[0].newUri.toString(true) } ); - if (!editsForFileRename) { + const edits = editsForFileRename?.documentChanges?.filter(TextDocumentEdit.is); + if (!edits) { + return; + } + + // If a file move/rename of a TS/JS file results in TS/JS file updates only, + // skip because the TypeScript LS will take care of it. + if ( + (oldUri.endsWith('.ts') || oldUri.endsWith('.js')) && + !edits.some((change) => change.textDocument.uri.endsWith('.svelte')) + ) { return; } const workspaceEdit = new WorkspaceEdit(); // Renaming a file should only result in edits of existing files - editsForFileRename.documentChanges?.filter(TextDocumentEdit.is).forEach((change) => + edits.forEach((change) => change.edits.forEach((edit) => { workspaceEdit.replace( Uri.parse(change.textDocument.uri), diff --git a/packages/svelte-vscode/src/tsplugin.ts b/packages/svelte-vscode/src/tsplugin.ts index 2a474c586..066b2afe6 100644 --- a/packages/svelte-vscode/src/tsplugin.ts +++ b/packages/svelte-vscode/src/tsplugin.ts @@ -4,12 +4,12 @@ export class TsPlugin { private enabled: boolean; constructor(context: ExtensionContext) { - this.enabled = this.getEnabledState(); + this.enabled = TsPlugin.isEnabled(); this.toggleTsPlugin(this.enabled); context.subscriptions.push( workspace.onDidChangeConfiguration(() => { - const enabled = this.getEnabledState(); + const enabled = TsPlugin.isEnabled(); if (enabled !== this.enabled) { this.enabled = enabled; this.toggleTsPlugin(this.enabled); @@ -18,7 +18,7 @@ export class TsPlugin { ); } - private getEnabledState(): boolean { + static isEnabled(): boolean { return workspace.getConfiguration('svelte').get('enable-ts-plugin') ?? false; } diff --git a/packages/typescript-plugin/src/language-service/index.ts b/packages/typescript-plugin/src/language-service/index.ts index b43f69526..39b662b9e 100644 --- a/packages/typescript-plugin/src/language-service/index.ts +++ b/packages/typescript-plugin/src/language-service/index.ts @@ -9,6 +9,7 @@ import { decorateDiagnostics } from './diagnostics'; import { decorateFindReferences } from './find-references'; import { decorateGetImplementation } from './implementation'; import { decorateRename } from './rename'; +import { decorateUpdateImports } from './update-imports'; const sveltePluginPatchSymbol = Symbol('sveltePluginPatchSymbol'); @@ -41,6 +42,7 @@ function decorateLanguageServiceInner( decorateCompletions(ls, logger); decorateGetDefinition(ls, snapshotManager, logger); decorateGetImplementation(ls, snapshotManager, logger); + decorateUpdateImports(ls, snapshotManager, logger); return ls; } diff --git a/packages/typescript-plugin/src/language-service/update-imports.ts b/packages/typescript-plugin/src/language-service/update-imports.ts new file mode 100644 index 000000000..18a7a1e1c --- /dev/null +++ b/packages/typescript-plugin/src/language-service/update-imports.ts @@ -0,0 +1,34 @@ +import type ts from 'typescript/lib/tsserverlibrary'; +import { Logger } from '../logger'; +import { SvelteSnapshotManager } from '../svelte-snapshots'; +import { isNotNullOrUndefined, isSvelteFilePath } from '../utils'; + +export function decorateUpdateImports( + ls: ts.LanguageService, + snapshotManager: SvelteSnapshotManager, + logger: Logger +): void { + const getEditsForFileRename = ls.getEditsForFileRename; + ls.getEditsForFileRename = (oldFilePath, newFilePath, formatOptions, preferences) => { + const renameLocations = getEditsForFileRename( + oldFilePath, + newFilePath, + formatOptions, + preferences + ); + // If a file move/rename of a TS/JS file results a Svelte file change, + // the Svelte extension will notice that, too, and adjusts the same imports. + // This results in duplicate adjustments which can break imports in some cases. + // Therefore don't do any updates in this case and let the Svelte extension handle that. + const containsSvelteFile = renameLocations?.some((renameLocation) => { + return isSvelteFilePath(renameLocation.fileName); + }); + if (containsSvelteFile) { + logger.debug( + 'File rename also touched Svelte file -> let Svelte extension handle that' + ); + return []; + } + return renameLocations; + }; +} From 1db960c626330206b7b29ae4cff127d3945fae8d Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 3 May 2022 21:54:18 +0200 Subject: [PATCH 2/3] more robust change of files --- packages/svelte-vscode/src/extension.ts | 25 ++++++++++--------- .../src/language-service/update-imports.ts | 15 +++-------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/packages/svelte-vscode/src/extension.ts b/packages/svelte-vscode/src/extension.ts index 9e9225a0e..40fdb689b 100644 --- a/packages/svelte-vscode/src/extension.ts +++ b/packages/svelte-vscode/src/extension.ts @@ -357,19 +357,20 @@ function addRenameFileListener(getLS: () => LanguageClient) { return; } - // If a file move/rename of a TS/JS file results in TS/JS file updates only, - // skip because the TypeScript LS will take care of it. - if ( - (oldUri.endsWith('.ts') || oldUri.endsWith('.js')) && - !edits.some((change) => change.textDocument.uri.endsWith('.svelte')) - ) { - return; - } - const workspaceEdit = new WorkspaceEdit(); // Renaming a file should only result in edits of existing files - edits.forEach((change) => + edits.forEach((change) => { + const isTsOrJsFile = + change.textDocument.uri.endsWith('.ts') || + change.textDocument.uri.endsWith('.js'); + change.edits.forEach((edit) => { + // If the moved/renamed file is a TS/JS file, skip all TS/JS updates + // because the TypeScript LS will take care of it. + if (isTsOrJsFile && !edit.newText.endsWith('.svelte')) { + return; + } + workspaceEdit.replace( Uri.parse(change.textDocument.uri), new Range( @@ -378,8 +379,8 @@ function addRenameFileListener(getLS: () => LanguageClient) { ), edit.newText ); - }) - ); + }); + }); workspace.applyEdit(workspaceEdit); } ); diff --git a/packages/typescript-plugin/src/language-service/update-imports.ts b/packages/typescript-plugin/src/language-service/update-imports.ts index 18a7a1e1c..e34d84e04 100644 --- a/packages/typescript-plugin/src/language-service/update-imports.ts +++ b/packages/typescript-plugin/src/language-service/update-imports.ts @@ -1,7 +1,7 @@ import type ts from 'typescript/lib/tsserverlibrary'; import { Logger } from '../logger'; import { SvelteSnapshotManager } from '../svelte-snapshots'; -import { isNotNullOrUndefined, isSvelteFilePath } from '../utils'; +import { isSvelteFilePath } from '../utils'; export function decorateUpdateImports( ls: ts.LanguageService, @@ -19,16 +19,9 @@ export function decorateUpdateImports( // If a file move/rename of a TS/JS file results a Svelte file change, // the Svelte extension will notice that, too, and adjusts the same imports. // This results in duplicate adjustments which can break imports in some cases. - // Therefore don't do any updates in this case and let the Svelte extension handle that. - const containsSvelteFile = renameLocations?.some((renameLocation) => { - return isSvelteFilePath(renameLocation.fileName); + // Therefore don't do any updates of Svelte files and let the Svelte extension handle that. + return renameLocations?.filter((renameLocation) => { + return !isSvelteFilePath(renameLocation.fileName); }); - if (containsSvelteFile) { - logger.debug( - 'File rename also touched Svelte file -> let Svelte extension handle that' - ); - return []; - } - return renameLocations; }; } From e67754ea69028e3f91fe45bd231e4b00ce94263c Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 4 May 2022 13:59:34 +0200 Subject: [PATCH 3/3] man this is hard to think through --- packages/svelte-vscode/src/extension.ts | 30 ++++++++++++++++--- .../src/language-service/update-imports.ts | 11 +++++-- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/packages/svelte-vscode/src/extension.ts b/packages/svelte-vscode/src/extension.ts index 40fdb689b..9c7e9700b 100644 --- a/packages/svelte-vscode/src/extension.ts +++ b/packages/svelte-vscode/src/extension.ts @@ -358,19 +358,41 @@ function addRenameFileListener(getLS: () => LanguageClient) { } const workspaceEdit = new WorkspaceEdit(); - // Renaming a file should only result in edits of existing files + // We need to take into account multiple cases: + // - A Svelte file is moved/renamed + // -> all updates will be related to that Svelte file, do that here. The TS LS won't even notice the update + // - A TS/JS file is moved/renamed + // -> all updates will be related to that TS/JS file + // -> let the TS LS take care of these updates in TS/JS files, do Svelte file updates here + // - A folder with TS/JS AND Svelte files is moved/renamed + // -> all Svelte file updates are handled here + // -> all TS/JS file updates that consist of only TS/JS import updates are handled by the TS LS + // -> all TS/JS file updates that consist of only Svelte import updates are handled here + // -> all TS/JS file updates that are mixed are handled here, but also possibly by the TS LS + // if the TS plugin doesn't prevent it. This trades risk of broken updates with certainty of missed updates edits.forEach((change) => { const isTsOrJsFile = change.textDocument.uri.endsWith('.ts') || change.textDocument.uri.endsWith('.js'); + const containsSvelteImportUpdate = change.edits.some((edit) => + edit.newText.endsWith('.svelte') + ); + if (isTsOrJsFile && !containsSvelteImportUpdate) { + return; + } change.edits.forEach((edit) => { - // If the moved/renamed file is a TS/JS file, skip all TS/JS updates - // because the TypeScript LS will take care of it. - if (isTsOrJsFile && !edit.newText.endsWith('.svelte')) { + if ( + isTsOrJsFile && + !TsPlugin.isEnabled() && + !edit.newText.endsWith('.svelte') + ) { + // TS plugin enabled -> all mixed imports are handled here + // TS plugin disabled -> let TS/JS path updates be handled by the TS LS, Svelte here return; } + // Renaming a file should only result in edits of existing files workspaceEdit.replace( Uri.parse(change.textDocument.uri), new Range( diff --git a/packages/typescript-plugin/src/language-service/update-imports.ts b/packages/typescript-plugin/src/language-service/update-imports.ts index e34d84e04..c0de3d3d5 100644 --- a/packages/typescript-plugin/src/language-service/update-imports.ts +++ b/packages/typescript-plugin/src/language-service/update-imports.ts @@ -18,10 +18,15 @@ export function decorateUpdateImports( ); // If a file move/rename of a TS/JS file results a Svelte file change, // the Svelte extension will notice that, too, and adjusts the same imports. - // This results in duplicate adjustments which can break imports in some cases. - // Therefore don't do any updates of Svelte files and let the Svelte extension handle that. + // This results in duplicate adjustments or race conditions with conflicting text spans + // which can break imports in some cases. + // Therefore don't do any updates of Svelte files and and also no updates of mixed TS files + // and let the Svelte extension handle that. return renameLocations?.filter((renameLocation) => { - return !isSvelteFilePath(renameLocation.fileName); + return ( + !isSvelteFilePath(renameLocation.fileName) && + !renameLocation.textChanges.some((change) => change.newText.endsWith('.svelte')) + ); }); }; }