(fix) prevent duplicate import updates (#1466)

The TS plugin and the Svelte extension want to update the same locations after a file move/rename in some cases; prevent that
#1461

Author: dummdidumm

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<WorkspaceEdit | null> {
+        // 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) {

packages/svelte-vscode/src/extension.ts

@@ -352,14 +352,47 @@ function addRenameFileListener(getLS: () => LanguageClient) {
                         newUri: evt.files[0].newUri.toString(true)
                     }
                 );
-                if (!editsForFileRename) {
+                const edits = editsForFileRename?.documentChanges?.filter(TextDocumentEdit.is);
+                if (!edits) {
                     return;
                 }
 
                 const workspaceEdit = new WorkspaceEdit();
-                // Renaming a file should only result in edits of existing files
-                editsForFileRename.documentChanges?.filter(TextDocumentEdit.is).forEach((change) =>
+                // 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 (
+                            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(
@@ -368,8 +401,8 @@ function addRenameFileListener(getLS: () => LanguageClient) {
                             ),
                             edit.newText
                         );
-                    })
-                );
+                    });
+                });
                 workspace.applyEdit(workspaceEdit);
             }
         );

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<boolean>('enable-ts-plugin') ?? false;
     }
 

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;
 }
 

packages/typescript-plugin/src/language-service/update-imports.ts

@@ -0,0 +1,32 @@
+import type ts from 'typescript/lib/tsserverlibrary';
+import { Logger } from '../logger';
+import { SvelteSnapshotManager } from '../svelte-snapshots';
+import { 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 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) &&
+                !renameLocation.textChanges.some((change) => change.newText.endsWith('.svelte'))
+            );
+        });
+    };
+}