[fix] harden migration around imports (#5828)

* harden declared logic around imports: remove type only load imports, handle imports with same name as load exports, bail on export *, handle undefined declared map.

* changeset

.changeset/cuddly-ads-kick.md

@@ -0,0 +1,5 @@
+---
+'svelte-migrate': patch
+---
+
+handle more import cases

packages/migrate/migrations/routes/index.js

@@ -9,7 +9,7 @@ import { migrate_scripts } from './migrate_scripts/index.js';
 import { migrate_page } from './migrate_page_js/index.js';
 import { migrate_page_server } from './migrate_page_server/index.js';
 import { migrate_server } from './migrate_server/index.js';
-import { adjust_imports, bail, dedent, move_file, relative, task } from './utils.js';
+import { adjust_imports, bail, move_file, relative, task } from './utils.js';
 
 export async function migrate() {
 	if (!fs.existsSync('svelte.config.js')) {

packages/migrate/migrations/routes/migrate_scripts/index.js

@@ -1,5 +1,13 @@
 import ts from 'typescript';
-import { adjust_imports, guess_indent, comment, error, dedent, parse } from '../utils.js';
+import {
+	adjust_imports,
+	guess_indent,
+	comment,
+	error,
+	dedent,
+	parse,
+	except_str
+} from '../utils.js';
 import * as TASKS from '../tasks.js';
 
 /**
@@ -14,37 +22,57 @@ export function migrate_scripts(content, is_error, moved) {
 	// instance script
 	const main = content.replace(
 		/<script([^]*?)>([^]+?)<\/script>(\n*)/g,
-		(match, attrs, content, whitespace) => {
-			const indent = guess_indent(content) ?? '';
+		(match, attrs, contents, whitespace) => {
+			const indent = guess_indent(contents) ?? '';
 
 			if (moved) {
-				content = adjust_imports(content);
+				contents = adjust_imports(contents);
 			}
 
 			if (/context=(['"])module\1/.test(attrs)) {
 				// special case — load is no longer supported in error
 				if (is_error) {
 					const body = `\n${indent}${error('Replace error load function', '3293209')}\n${comment(
-						content,
+						contents,
 						indent
 					)}`;
 
 					return `<script${attrs}>${body}</script>${whitespace}`;
 				}
 
-				module = dedent(content.replace(/^\n/, ''));
+				module = dedent(contents.replace(/^\n/, ''));
 
-				const declared = find_declarations(content);
-				declared.delete('load');
-				declared.delete('router');
-				declared.delete('hydrate');
-				declared.delete('prerender');
+				const declared = find_declarations(contents);
+				const delete_var = (/** @type {string } */ key) => {
+					const declaration = declared?.get(key);
+					if (declaration && !declaration.import) {
+						declared.delete(key);
+					}
+				};
+				delete_var('load');
+				delete_var('router');
+				delete_var('hydrate');
+				delete_var('prerender');
+				const delete_kit_type = (/** @type {string } */ key) => {
+					const declaration = declared?.get(key);
+					if (
+						declaration &&
+						declaration.import?.type_only &&
+						declaration.import.from === '@sveltejs/kit' &&
+						!new RegExp(`\\W${key}\\W`).test(except_str(content, match))
+					) {
+						declared.delete(key);
+					}
+				};
+				delete_kit_type('Load');
+				delete_kit_type('LoadEvent');
+				delete_kit_type('LoadOutput');
 
-				if (declared.size > 0) {
+				if (!declared || declared.size > 0) {
 					const body = `\n${indent}${error(
 						'Check code was safely removed',
 						TASKS.PAGE_MODULE_CTX
-					)}\n${comment(content, indent)}`;
+					)}\n${comment(contents, indent)}`;
 
 					return `<script${attrs}>${body}</script>${whitespace}`;
 				}
@@ -53,12 +81,12 @@ export function migrate_scripts(content, is_error, moved) {
 				return '';
 			}
 
-			if (!is_error && /export/.test(content)) {
-				content = `\n${indent}${error('Add data prop', TASKS.PAGE_DATA_PROP)}\n${content}`;
+			if (!is_error && /export/.test(contents)) {
+				contents = `\n${indent}${error('Add data prop', TASKS.PAGE_DATA_PROP)}\n${contents}`;
 				// Possible TODO: migrate props to data.prop, or suggest $: ({propX, propY, ...} = data);
 			}
 
-			return `<script${attrs}>${content}</script>${whitespace}`;
+			return `<script${attrs}>${contents}</script>${whitespace}`;
 		}
 	);
 
@@ -70,37 +98,50 @@ function find_declarations(content) {
 	const file = parse(content);
 	if (!file) return;
 
-	const declared = new Set();
+	/** @type {Map<string, {name: string, import: {from: string, type_only: boolean}}>} */
+	const declared = new Map();
+	/**
+	 * @param {string} name
+	 * @param {{from: string, type_only: boolean}} [import_def]
+	 */
+	function add(name, import_def) {
+		declared.set(name, { name, import: import_def });
+	}
 
 	for (const statement of file.ast.statements) {
 		if (ts.isImportDeclaration(statement) && statement.importClause) {
+			let type_only = statement.importClause.isTypeOnly;
+			const from = ts.isStringLiteral(statement.moduleSpecifier) && statement.moduleSpecifier.text;
+
 			if (statement.importClause.name) {
-				declared.add(statement.importClause.name.text);
+				add(statement.importClause.name.text, { from, type_only });
 			}
 
 			const bindings = statement.importClause.namedBindings;
 
 			if (bindings) {
 				if (ts.isNamespaceImport(bindings)) {
-					declared.add(bindings.name.text);
+					add(bindings.name.text, { from, type_only });
 				} else {
 					for (const binding of bindings.elements) {
-						declared.add(binding.name.text);
+						add(binding.name.text, { from, type_only: type_only || binding.isTypeOnly });
 					}
 				}
 			}
 		} else if (ts.isVariableStatement(statement)) {
 			for (const declaration of statement.declarationList.declarations) {
 				if (ts.isIdentifier(declaration.name)) {
-					declared.add(declaration.name.text);
+					add(declaration.name.text);
 				} else {
 					return; // bail out if it's not a simple variable
 				}
 			}
 		} else if (ts.isFunctionDeclaration(statement) || ts.isClassDeclaration(statement)) {
 			if (ts.isIdentifier(statement.name)) {
-				declared.add(statement.name.text);
+				add(statement.name.text);
 			}
+		} else if (ts.isExportDeclaration(statement) && !statement.exportClause) {
+			return; // export * from '..' -> bail
 		}
 	}
 

packages/migrate/migrations/routes/migrate_scripts/samples.md

@@ -125,3 +125,123 @@
 
 <Foo>{sry}</Foo>
 ```
+
+## Module context with type imports only
+
+```svelte before
+<script context="module">
+	import type { Load, LoadEvent, LoadOutput } from '@sveltejs/kit';
+	export function load() {
+		return {
+			props: {
+				sry: 'not anymore'
+			}
+		}
+	}
+</script>
+```
+
+```svelte after
+```
+
+## Module context with type imports only but used in instance script
+
+```svelte before
+<script context="module">
+	import type { Load, LoadEvent, LoadOutput } from '@sveltejs/kit';
+	export function load() {
+		return {
+			props: {
+				sry: 'not anymore'
+			}
+		}
+	}
+</script>
+
+<script>
+	const whywouldyoudothis: Load = 'I dont know lol';
+</script>
+```
+
+```svelte after
+<script context="module">
+	throw new Error("@migration task: Check code was safely removed (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292722)");
+
+	// import type { Load, LoadEvent, LoadOutput } from '@sveltejs/kit';
+	// export function load() {
+	// 	return {
+	// 		props: {
+	// 			sry: 'not anymore'
+	// 		}
+	// 	}
+	// }
+</script>
+
+<script>
+	const whywouldyoudothis: Load = 'I dont know lol';
+</script>
+```
+
+## Module context with export * from '..'
+
+```svelte before
+<script context="module">
+	export * from './somewhere';
+</script>
+```
+
+```svelte after
+<script context="module">
+	throw new Error("@migration task: Check code was safely removed (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292722)");
+
+	// export * from './somewhere';
+</script>
+```
+
+## Module context with named imports
+
+```svelte before
+<script context="module">
+	import { bar } from './somewhere';
+</script>
+
+<script>
+	let foo = bar;
+</script>
+```
+
+```svelte after
+<script context="module">
+	throw new Error("@migration task: Check code was safely removed (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292722)");
+
+	// import { bar } from './somewhere';
+</script>
+
+<script>
+	let foo = bar;
+</script>
+```
+
+## Module context with named imports that have same name as a load option
+
+```svelte before
+<script context="module">
+	import { router } from './somewhere';
+</script>
+
+<script>
+	let foo = router;
+</script>
+```
+
+```svelte after
+<script context="module">
+	throw new Error("@migration task: Check code was safely removed (https://github.com/sveltejs/kit/discussions/5774#discussioncomment-3292722)");
+
+	// import { router } from './somewhere';
+</script>
+
+<script>
+	let foo = router;
+</script>
+```

packages/migrate/migrations/routes/utils.js

@@ -402,6 +402,16 @@ export function parse(content) {
 	}
 }
 
+/**
+ * @param {string} content
+ * @param {string} except
+ */
+export function except_str(content, except) {
+	const start = content.indexOf(except);
+	const end = start + except.length;
+	return content.substring(0, start) + content.substring(end);
+}
+
 /** @param {string} test_file */
 export function read_samples(test_file) {
 	const markdown = fs.readFileSync(new URL('./samples.md', test_file), 'utf8');